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 osm_config.yaml #822

Merged
merged 22 commits into from
Aug 28, 2023
Merged

Conversation

Tomkourou
Copy link
Contributor

@Tomkourou Tomkourou commented Aug 4, 2023

Closes #814

Changes proposed in this Pull Request

  • Deletes the config_osm.py
  • adds an osm_config.yaml to configs folder.
  • Replaces all config_osm.py dependencies with osm_config.yaml

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.

@Tomkourou Tomkourou marked this pull request as ready for review August 4, 2023 23:24
@davide-f
Copy link
Member

davide-f commented Aug 5, 2023

Amazing @Tomkourou !!!
I'll add some comments :)

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! :)
I've added some comments. The main issue seems the windows CI to be breaking though, the rest are minor comments.
Thanks for addressing this! :D

configs/osm_config.yaml Show resolved Hide resolved
configs/osm_config.yaml Show resolved Hide resolved
scripts/_helpers.py Outdated Show resolved Hide resolved
@Tomkourou Tomkourou requested a review from davide-f August 8, 2023 14:12
@Tomkourou
Copy link
Contributor Author

For some reason I cannot resolve the merge conflict.

@davide-f
Copy link
Member

davide-f commented Aug 8, 2023

Mmm that's weird: the merge conflict seems to relate to the config_osm_data.py file that is deleted. The merge conflict should be a basically empty commit.
You may need to resolve conflicts locally using raw git.
There are instructions on how to do that here (make a copy of the folder before and/or a copy branch)
Have you tried the git approach?

configs/osm_config.yaml Outdated Show resolved Hide resolved
configs/osm_config.yaml Show resolved Hide resolved
scripts/_helpers.py Outdated Show resolved Hide resolved
@Tomkourou
Copy link
Contributor Author

Mmm that's weird: the merge conflict seems to relate to the config_osm_data.py file that is deleted. The merge conflict should be a basically empty commit.
You may need to resolve conflicts locally using raw git.
There are instructions on how to do that here (make a copy of the folder before and/or a copy branch)
Have you tried the git approach?

What do you mean by 'here'? ^^ not sure the approach you are referring to.

@davide-f
Copy link
Member

davide-f commented Aug 9, 2023

Mmm that's weird: the merge conflict seems to relate to the config_osm_data.py file that is deleted. The merge conflict should be a basically empty commit.
You may need to resolve conflicts locally using raw git.
There are instructions on how to do that here (make a copy of the folder before and/or a copy branch)
Have you tried the git approach?

What do you mean by 'here'? ^^ not sure the approach you are referring to.

Here in github, if you go and click on resolve conflicts, you should see a list of git codes that can be used as a draft for merging the changes locally in your computer.
Before submitting the first review I was able to read those lines, but now no more-

@Tomkourou
Copy link
Contributor Author

Should be good to go :)

@davide-f
Copy link
Member

Mmm the CI is not working, maybe something has gone wrong in the merge.
May you Verify?

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.

Cool @Tomkourou :)
There are some old comments that have no answer. I've added additional comments there.
Could you please check the github page?

Would be nice also to know why this happens to help all users.
Many thanks :D

scripts/_helpers.py Outdated Show resolved Hide resolved
scripts/_helpers.py Outdated Show resolved Hide resolved
scripts/_helpers.py Outdated Show resolved Hide resolved
scripts/_helpers.py Outdated Show resolved Hide resolved
scripts/download_osm_data.py Outdated Show resolved Hide resolved
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 @Tomkourou :D
The PR is ready, potentially, the description of read_osm_config may be improved to detail the returned objs in function of the required inputs. Currently the return value is described as being a tuple but that's not always.

Except for this detail that is a non-blocking element, please, fill the authorization on releasing with AGPL and the release_note.
Especially the AGPL is needed for merging...

Many thanks :D

@Tomkourou
Copy link
Contributor Author

Just updated the release notes and agreed to the AGPL. @davide-f I won't have time probably today to improve the docstrings although I see the value. Happy for you to take it in hand if you have the capacity :)

@Tomkourou
Copy link
Contributor Author

Just updated the release notes and agreed to the AGPL. @davide-f I won't have time probably today to improve the docstrings although I see the value. Happy for you to take it in hand if you have the capacity :)

Scratch that it's done. @davide-f

@davide-f
Copy link
Member

Super @Tomkourou ! :D
Ready to merge :)

@davide-f davide-f merged commit 6e6dbf6 into pypsa-meets-earth:main Aug 28, 2023
3 checks passed
@Tomkourou Tomkourou deleted the remove-config-osm branch August 28, 2023 17:33
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.

Completely revise the handling of osm configs
2 participants