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

Unit test build shapes and minor changes to test_base_network.py #1466

Merged
merged 21 commits into from
Jan 9, 2025

Conversation

finozzifa
Copy link
Collaborator

@finozzifa finozzifa commented Dec 19, 2024

Closes # (if applicable).

Changes proposed in this Pull Request

The pull request proposes the following unit tests:

test/test_build_shapes.py:

  • test_simplify_polys
  • test_countries
  • test_eez
  • test_country_cover

conftest.py:

  • I added the italy_shape fixture. It replaces the test/test_data/italy_shape.geojson file (which in fact has been deleted).
  • I have added the download_natural_earth fixture
  • I have added the download_eez fixture

test/test_base_network.py:

  • I replaced italy_shape.geojson with the fixture italy_shape

Checklist

  • I tested my contribution locally and it works as intended.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to envs/environment.yaml.
  • Changes in configuration options are added in config/config.default.yaml.
  • Changes in configuration options are documented in doc/configtables/*.csv.
  • Sources of newly added data are documented in doc/data_sources.rst.
  • A release note doc/release_notes.rst is added.

@finozzifa finozzifa changed the title Unit test build shapes Unit test build shapes and minor changes to test_base_network.py Dec 20, 2024
@finozzifa finozzifa marked this pull request as ready for review January 8, 2025 17:06
Copy link
Member

@lkstrp lkstrp left a comment

Choose a reason for hiding this comment

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

Thank you again for your work on the unit tests!

I have concerns about how we're going to handle data retrieval here.
I'm not sure if we want to open up another section where we define urls and retrieve them with some extra logic? I think we should keep this out of the unit tests.

The retrieval is not very stable and breaks from time to time. And it is all ready and will be even more tedious to fix it twice. Especially since we have no common input data management (yet). Also this slows down the unit tests quite a bit, isnt't it?

But also not sure how to handle this instead. What do you think?

@finozzifa
Copy link
Collaborator Author

finozzifa commented Jan 9, 2025

hey @lkstrp,

thanks for your comment :)

I can guide you through my reasoning.

For the unit testing we need many input files. I do not know exactly which ones are actually needed and what their size will be in the end. This rules out the possibility of storing such inputs (we do not know how much space we need to allocate for that).

My focus now is to write as many unit tests as possible in the least time as possible.

Hence, I decided to write the fixtures that download just the data that we need as a temporary and light solution.

My suggestion would be to go forward with the solution currently at hand (a.k.a. the fixtures) and once we have a better idea of how many inputs we need and what their sizes will be, we can then find better solutions.

Thanks :)

@lkstrp
Copy link
Member

lkstrp commented Jan 9, 2025

My focus now is to write as many unit tests as possible in the least time as possible.

Let's go! Ok, then we fix all potential input data issues once they occur or move the whole logic to some input data management tool at some point

@lkstrp lkstrp merged commit 638e001 into PyPSA:master Jan 9, 2025
12 checks passed
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