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

Create statistics #579

Merged
merged 49 commits into from
Feb 14, 2023
Merged

Create statistics #579

merged 49 commits into from
Feb 14, 2023

Conversation

davide-f
Copy link
Member

@davide-f davide-f commented Jan 25, 2023

Closes # (if applicable).

Changes proposed in this Pull Request

This PR aims at creating a new rule (make_statistics) that create statistics on the overall workflow.
This PR requires changes from #543 so first the #543 must be reviewed and merged and then this one can follow.

Checklist

  • 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 ready for review January 25, 2023 17:37
@davide-f
Copy link
Member Author

This PR is ready for preliminary reviews but not for complete merge as it requires #543 to be merged first (some commits seen here are those from #543)

@davide-f davide-f force-pushed the create_statistics branch 2 times, most recently from ecb6d7d to edb7bf7 Compare February 13, 2023 18:57
@davide-f davide-f requested a review from pz-max February 13, 2023 19:00
@davide-f
Copy link
Member Author

@pz-max I think this PR is ready for review.
It contains several improvements that may be better merged and work on them later, yet appropriate review is needed

@pz-max
Copy link
Member

pz-max commented Feb 14, 2023

@davide-f happy to do that. I see several good improvements and great code (as always)
Do you think we could create a small notebook in the documentation repo to demonstrate the make_statistic stuff?

I am thinking about a super small notebook:

  • instructions how to modify the config tutorial/ test case for Nigeria
  • instructions what command to use to run the whole stats stuff
  • some tables with the stats maybe with little explanations

This would also help review a lot

@davide-f
Copy link
Member Author

I'd leave that for a subsequent PR maybe later on when this scenario generation and statistics is fully stable and validated with the paper.
What do you think?

@pz-max
Copy link
Member

pz-max commented Feb 14, 2023

We agreed:

  • I do a code review
  • @davide-f adds a docstring on use
  • we add an issue in the documentation repo that aims to add a jupyter notebook on the make statistics stuff

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.

Looks good to me. Main comments:

  • check if new created _mock_snakemake function can be avoided by passing snakemake objects over the function
  • PEP 257
  • Optional, little os to Pathlib switch at some locations

scripts/_helpers.py Show resolved Hide resolved
scripts/clean_osm_data.py Outdated Show resolved Hide resolved
scripts/make_statistics.py Show resolved Hide resolved
scripts/make_statistics.py Outdated Show resolved Hide resolved
scripts/make_statistics.py Outdated Show resolved Hide resolved
scripts/make_statistics.py Outdated Show resolved Hide resolved
scripts/make_statistics.py Show resolved Hide resolved
scripts/make_statistics.py Show resolved Hide resolved
scripts/make_statistics.py Show resolved Hide resolved
scripts/make_statistics.py Show resolved Hide resolved
@davide-f
Copy link
Member Author

davide-f commented Feb 14, 2023

@pz-max if you like to test the complete run_all_scenarios.
You make clone this PR and run:
snakemake -j1 run_all_scenarios
I've added a test case for nigeria in the configs/scenarios so to be used as an example

Be aware that this will run the default configuration though

@pz-max
Copy link
Member

pz-max commented Feb 14, 2023

@pz-max if you like to test the complete run_all_scenarios. You make clone this PR and run: snakemake -j1 run_all_scenarios I've added a test case for nigeria in the configs/scenarios so to be used as an example

Be aware that this will run the default configuration though

  • Probably nice to make it run the tutorial or not? (for testing etc)
  • Checking the CI, the test needs to be adapted

Ready to merge if all tests are passing

@davide-f
Copy link
Member Author

davide-f commented Feb 14, 2023

@pz-max if you like to test the complete run_all_scenarios. You make clone this PR and run: snakemake -j1 run_all_scenarios I've added a test case for nigeria in the configs/scenarios so to be used as an example
Be aware that this will run the default configuration though

  • Probably nice to make it run the tutorial or not? (for testing etc)
  • Checking the CI, the test needs to be adapted

Ready to merge if all tests are passing

You may try locally to run it with the tutorial. I never tested it but it may work.
Just:

  1. copy the tutorial config
  2. run snakemake -j1 run_all_scenarios

the default configuration file that is used as a base case for the run_scenarios is automatically chosen using the tutorial flag.
So it may work as is now.

Currently, I have the folder filled with world data in default configuration, I cannot mess it up...

@davide-f
Copy link
Member Author

The CI works and there the run_scenario is already implemented

@pz-max pz-max self-requested a review February 14, 2023 14:01
@pz-max pz-max merged commit 6c1d2d3 into pypsa-meets-earth:main Feb 14, 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.

2 participants