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 - layer_id not resetting after each loop #663

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

GridGrapher
Copy link
Contributor

Closes # (if applicable).

Changes proposed in this Pull Request

When running build_shapes for ['earth'] there were inconsistencies in the gadm_layer_id of countries, see below:

gadm_layer_id = 1

Earth_layer_id_1

gadm_layer_id = 10

Earth_layer_id_10

By going through the code it seems layer_id wasn't being set to the global gdam_layer_id in get_GADM_layer.
This would result in layer_id being set to 0 once countries such as 'NU' were reached, then all following countries would use 0 instead of the original gadm_layer_id.

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.

@pz-max
Copy link
Member

pz-max commented Apr 2, 2023

Great PR and description. Looks like a nice bug that you catched @GridGrapher. Changes look good to me. Since @davide-f currently plays more around with the global stability, I will leave the approval to him.

@davide-f
Copy link
Member

davide-f commented Apr 3, 2023

Thanks @GridGrapher , that's a very nice catch!

I see that for gadm layer level 10, the shapes of some countries collapse to level 0, rather, we would like to have the maximum of each level.
Could you explain if that's the case? The code seems fine, but I'm wondering the plot
I'll properly review the code today

@davide-f
Copy link
Member

davide-f commented Apr 3, 2023

This PR could be merged as-is; however, it would conflict with the history of the PR to revise add_population_data as this commit is different from the one of the other PR.

To avoid duplication, you may reset this PR to the main and do a cherry-pick on this PR

git reset --hard b5a214c
git cherry-pick ad38dae
git push --force

ad38dae should be the commit of the other add population PR and by doing this the two wouldn't be conflicting once merged

@GridGrapher
Copy link
Contributor Author

GridGrapher commented Apr 3, 2023

The adjustment makes a lot of sense to avoid merge conflicts, I've implemented the commands and it should now be correct.

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.

Thanks @GridGrapher . This will be merged after the tests are passing :)

@davide-f
Copy link
Member

davide-f commented Apr 3, 2023

Not sure why but it doesn't seems the right commit has been stored, @pz-max the code is ok, if the commit does not match a duplicate commit appears and maybe few merge conflicts

@GridGrapher
Copy link
Contributor Author

GridGrapher commented Apr 3, 2023

The code I'm using in the other branch includes engine="pyogrio" while it isn't imported in this environment.
I just spotted it looking over why it failed the test, probably best to revert this PR to a commit without "pyogrio" and deal with the merge locally in the other branch once I pull from main.

@pz-max
Copy link
Member

pz-max commented Apr 3, 2023

@GridGrapher yep, pyogrio needs to be removed. I am fine by dealing with the merge conflicts locally and think this can be merged. @davide-f has the last word/ will merge

@GridGrapher
Copy link
Contributor Author

I reset to without pyogrio, it should be straight forward to merge this into the other branch once this is on main, so I'll leave it up to @davide-f for the course of action.

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.

good to go and approved also by @davide-f

@pz-max pz-max merged commit a7db1fa into pypsa-meets-earth:main Apr 3, 2023
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