Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Western asia databundle #837

Merged
merged 12 commits into from
Sep 26, 2023

Conversation

Emre-Yorat89
Copy link
Contributor

@Emre-Yorat89 Emre-Yorat89 commented Aug 14, 2023

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
  • Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing @Emre-Yorat89.
This PR is almost ready to go, few comments only :)

@@ -348,3 +348,13 @@ databundles:
output: [cutouts/cutout-2013-era5.nc]
disable_by_opt:
build_cutout: [all]

bundle_cutouts_westernasia:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing @Emre-Yorat89 :D
This seems to be ok, the only comment I'd recommend is to add a comment on top of the bundle similarly to the others.
You may use Asia or Africa as templates.

@@ -9,7 +9,7 @@

# .. code:: yaml

# bundle_name: # name of the bundle
# _name: # name of the bundle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, restore bundle_name

@davide-f davide-f added this to the Version 0.3 milestone Aug 15, 2023
@ekatef
Copy link
Collaborator

ekatef commented Sep 8, 2023

Hey @Emre-Yorat89! As discussed, troubles with databundle you experience may be linked with the issue recently fixed by #844. In case updating the repo doesn't help, could you please add an issue on the problem you observe? It would be very helpful to increase stability of the model

destination: "cutouts"
urls:
gdrive: https://drive.google.com/file/d/110UA0WQnmRV-20cXxYNR8WHLnLti6Qip/view?usp=drive_link
output: [cutouts/western-asia-2013-era5.nc]
Copy link
Collaborator

@ekatef ekatef Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rename file to "cutout-2013-era5.nc" to make it consistent with other cutouts names?

@ekatef
Copy link
Collaborator

ekatef commented Sep 13, 2023

Hello @Emre-Yorat89! Thanks for looking into the issue with databundle download. It is in fact connected with "too many request" response from google server side (added a comment to #853 on that). That is quite annoying, but the good news is that the issue is not in any way caused by your PR. Feel free please to finalise it :)

You may absolutely replace the current link with an updated one and fix two comments above (one of which I have just corrected, sorry for the typo before!). Then, ready to merge, I think.

@ekatef
Copy link
Collaborator

ekatef commented Sep 15, 2023

Hey @Emre-Yorat89! Thanks a lot for updating the link and introducing the fixes.
Great work and ready to merge 🚀

The only point left is your consent to contribute under AGPL license which is used in the project. Would you mind to tick the first box in the check list to agree?

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Katia advised me that she is working on finalizing the PR.
I see that the git merge has been delicate.

To try to fix it, I'd recommend to:

  • copy this branch to avoid losing the changes
  • hard reset the branch before the merge that may have created issues. I believe a good commit may be:73d72a7783ce0985588df183439e65fab82bc20c . So the code is: git reset --hard 73d72a7 and then git push --force

This should restore the version before the merge went wrong

@ekatef
Copy link
Collaborator

ekatef commented Sep 23, 2023

Sorry @Emre-Yorat89 for taking a while to prepare PR for merge. It has been a need to add a release note and resolve merge conflicts.

@davide-f thanks a lot for git spells! It has been really helpful. My feeling is that now PR is ready. IC on mac fails due to an error which looks suspiciously similar to troubles with google drive... I'd wait until IC-mac work and then merge. Would you agree?

@davide-f
Copy link
Member

davide-f commented Sep 25, 2023

Hello @ekatef :)
As agreed, this PR is ready to squash and merge as the local check to verify that the google drive zip file is successfully automatically downloaded.
As that is done, the PR can be squashed and merged.

Congrats @Emre-Yorat89 and @ekatef :D

@ekatef
Copy link
Collaborator

ekatef commented Sep 26, 2023

Hello @ekatef :) As agreed, this PR is ready to squash and merge as the local check to verify that the google drive zip file is successfully automatically downloaded. As that is done, the PR can be squashed and merged.

Congrats @Emre-Yorat89 and @ekatef :D

Hello @davide-f! Thanks a lot for checking :D
I confirm that automated download works properly 🎉

@ekatef ekatef merged commit d545d82 into pypsa-meets-earth:main Sep 26, 2023
4 checks passed
@ekatef
Copy link
Collaborator

ekatef commented Sep 26, 2023

Merged 🙂
Thanks a lot @Emre-Yorat89 and @davide-f!
@Emre-Yorat89 congratulations with the finalised PR. Great work! 🥇

@Emre-Yorat89
Copy link
Contributor Author

Thank you @ekatef and @davide-f for your reviews and resolving the conflicts on the PR. I am happy it is finalized :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants