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

Fix workflow for countries with empty osm data #701

Merged
merged 22 commits into from
Jun 30, 2023

Conversation

davide-f
Copy link
Member

@davide-f davide-f commented May 2, 2023

Closes #642 , #641, #735

Changes proposed in this Pull Request

Add methods to handle cases when data from osm are empty.
This PR is built on top of #654

Checklist

  • I consent to the release of this PR's code under the GPLv3 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 envs/environment.docs.yaml.
  • 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.

@davide-f davide-f marked this pull request as draft May 2, 2023 14:17
@davide-f
Copy link
Member Author

davide-f commented May 2, 2023

Here I dropped the REUSE precommit because when working locally I do have a modified version of .gitignore and that is not read by the REUSE linter.
Therefore, the REUSE checked also ignored files and thus it was impossible to do any commit.
Since version checking shall be done once in a while and may apply only new scripts, I preferred to drop it here

@davide-f davide-f linked an issue May 2, 2023 that may be closed by this pull request
2 tasks
@pz-max
Copy link
Member

pz-max commented May 7, 2023

Here I dropped the REUSE precommit because when working locally I do have a modified version of .gitignore and that is not read by the REUSE linter.

Therefore, the REUSE checked also ignored files and thus it was impossible to do any commit.

Since version checking shall be done once in a while and may apply only new scripts, I preferred to drop it here

The reuse linting considers .gitignore. I also noted that it didn't always worked as intended, however, probably that was a mistake by the dev (me in this case). I made it work in the latest fix_earth PR.

I think if reuse is in the .pre-commit it won't hurt in case it works as intended. It's actually good, as we have one thing less to check (even though new file creation won't happen often)

@davide-f
Copy link
Member Author

davide-f commented May 7, 2023

I'm locally using the changes in #714 , not yet committed here as that is a cojoint activity with @ekatef and may be nice to have a cojoint PR

@davide-f
Copy link
Member Author

davide-f commented May 7, 2023

Here I dropped the REUSE precommit because when working locally I do have a modified version of .gitignore and that is not read by the REUSE linter.
Therefore, the REUSE checked also ignored files and thus it was impossible to do any commit.
Since version checking shall be done once in a while and may apply only new scripts, I preferred to drop it here

The reuse linting considers .gitignore. I also noted that it didn't always worked as intended, however, probably that was a mistake by the dev (me in this case). I made it work in the latest fix_earth PR.

I think if reuse is in the .pre-commit it won't hurt in case it works as intended. It's actually good, as we have one thing less to check (even though new file creation won't happen often)

I partially agree, but there is a problem:
Locally, I have folders backup, world and many results that are related to the world runs needed for papers and so on. To avoid problems, I have changed the .gitignore accordingly to avoid problems, but those changes are not committed.
REUSE ignores the files as specified in the gitignore in the repo, not the local version (it seems so).
Accordingly, every time I had to commit, REUSE was checking also those files that were locally ignored.
I had a ton of problems accordingly and I had alwas to go for no-verify using manual checks using precommit.

As a fix, I'll add to gitignore some additional folders to avoid such issue and keep the REUSE, but let's keep that into mind.

@pz-max
Copy link
Member

pz-max commented May 7, 2023

@davide-f it should consider the local .gitignore. So, I tend to believe that we don't use the .gitignore properly. In this SO post, I can see various avenues that we could try: https://stackoverflow.com/questions/2545602/how-to-git-ignore-subfolders-subdirectories

@davide-f
Copy link
Member Author

davide-f commented May 7, 2023

@davide-f it should consider the local .gitignore. So, I tend to believe that we don't use the .gitignore properly. In this SO post, I can see various avenues that we could try: https://stackoverflow.com/questions/2545602/how-to-git-ignore-subfolders-subdirectories

I agree that REUSE should consider the local gitignore, but it didn't seem the case in the tests that I did.
To bypass the issue, I pushed a new gitignore that works in my case: I put most of my debug files into backup* folders

@davide-f davide-f marked this pull request as ready for review May 7, 2023 14:07
@davide-f
Copy link
Member Author

davide-f commented May 7, 2023

With the combination of this PR and #714 All 193 countries are now running.
Some of them, however do not have a proper load from gegis as discussed in #707, however, that is not in the scope of this PR.

Priority should be given to merge #714 and then this PR can be merged.
Ideally, another complete country-wise run should be done before merging this PR but #714 is needed first.

@davide-f davide-f linked an issue Jun 9, 2023 that may be closed by this pull request
2 tasks
@davide-f
Copy link
Member Author

@pz-max or @ekatef could you please revise this quickly so that we can also fix pre-commit without conflicts to this?
In #779 the fix to pre-commit is ready but we need this to be merged first.
Then, I can create a new PR with the fix applied to the new main

Copy link
Member

@pz-max pz-max left a comment

Choose a reason for hiding this comment

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

Great changes and nothing to complain about

@pz-max pz-max merged commit 6340398 into pypsa-meets-earth:main Jun 30, 2023
3 checks passed
@davide-f davide-f deleted the fix_empty_data_countries branch October 1, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants