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

Refactoring changes and Pytest Integration #1209

Draft
wants to merge 75 commits into
base: main
Choose a base branch
from

Conversation

ekatef
Copy link
Member

@ekatef ekatef commented Nov 27, 2024

Restores #1083 with the updated repo structure.

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.

finozzifa and others added 30 commits June 5, 2024 00:05
…ne_connectetions, base_network, build_bus_regions
…py, cluster_network.py, download_osm_data.py, make_statistics.py
…r.py, plot_network.py, plot_summary.py, prepare_network.py, retrieve_databundle_light.py, simplify_network.py, solve_network.py
Os to pathlib and helpers.py method from pypsa-earth-sec
finozzifa and others added 25 commits September 4, 2024 16:41
* add unit test for gadm

* finalize unit test for gadm

* finalize unit test for gadm

* rename methods and remove unnecessary variables

* re-factor and test for country_cover

* add unit test for download_WorldPop_standard

* add unit test download_WorldPop_API

* update unit tests

* add unit test for add_population_data

* scripts/build_shapes.py

* add GDP data

* unit test load_gdp

* update environment packages

* remove one assert statement in test_get_gadm_shapes
* fix typo in main

* modify the way powerplant.csv is built

* unit test for replace_natural_gas_technology

* add unit test for add_power_plants

* reduce size of custom_powerplants.csv for Nigeria

* add documentation to add_power_plants unit test

* modify order of executions in test_add_power_plants

* remove unit tests for merge and false

* update changes

* update environments dependencies

* re-instate one part of the unit test

* PR requests

* remove bug
* add config options for prepare_network

* add new config

* initial tests

* unit test for emission_extractor

* add unit test for add_co2limit

* unit test for add_gaslimit

* add test for enforce_autarky

* add unit test for set_line_nom_max

* re-work test_add_emission_prices

* add unit tests for average_every_nhours and add_emission_prices

* modify prepare_network_options

* modify prepare_network_options

* replace cc.convert with two_2_three_digits_country

* remove import country_convert

* revert change on cc.convert

* change away from cc.convert

* fixtures for grid

* resolve conflicts

* pull request comments

* remove unnecessary params

* update geopandas version

* update environment files

* update environment files

* set upper limiti on powerplantmatching version
* Update release_notes.rst (pypsa-meets-earth#1104)

* Merge pull request pypsa-meets-earth#1086 from merge-pyspa-earth-sec

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Comment add_brownfield to bypass linter

* Add myopic test

* Revise irena and minor fixes

* Revise test name

* Add existing_heating

* implement review suggesstions

* bug fix in solve_network with reference case

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update submodule

* Add zeros for missing entries - aluminium

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* adding missing templates

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix MissingOutputException

* fix TypeError (pypsa-meets-earth#321)

* fix TypeError

* Update scripts/prepare_gas_network.py

Co-authored-by: Davide Fioriti <[email protected]>

---------

Co-authored-by: Davide Fioriti <[email protected]>

* solve pandas deprecations

* Add Params for Rule add_export.py

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to Rule build_base_energy_totals

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to Rule build_base_industry_totals

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to Rule build_cop_profiles

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to Rule build_heat_demand

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to build_industrial_distribution_key

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to build_industry_demand

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params and urban_percentage effect all Rules

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to build_ship_profile

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params build_solar_thermal_profiles

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to build_temperature_profiles

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to copy_config

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params make_summary

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add to override_respot + panning_horizons wildcard

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update Snakefile

* Update Snakefile

* Add Params to prepare_airports

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to prepare_gas_network

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Params to prepare_db and energy_totals

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update Snakefile

* Update add_export.py

* Add Params to build_population_layout

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* enhance the industry scripts and adapt the fuel aggregation

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove legacy EUR code

* remove legacy params form config

* remove legacy params form config

* Update build_base_industry_totals.py

* Update build_base_energy_totals.py

* omit double transpose

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove plotting from workflow

* remove plotting from prepare_gas_network

* Test routine: add Makefile
CI: consolidate ci.yaml files

* CI: bump cache number, remove os prefixes and labels
env: remove ipopt restriction

* Makefile: add tutorial yaml as additional config

* snakefile: use config.default as basis
ci: use tutorial config as secondary config

* snakefile: fix databundle config path

* debug: print out downloaded files

* snakefile: use renewables as bases for used cutouts

* Include scenario management

* Add scenario in tutorial

* Update submodule

* Add shared_cutouts

* Fix missing RDIR_PE

* fix IndexError (pypsa-meets-earth#326)

* fix IndexError

* '.' gets only added when len(id) > 3

* fix with layer_id

* Update scripts/prepare_gas_network.py

Co-authored-by: Davide Fioriti <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix index error when layer_id == 0

---------

Co-authored-by: Davide Fioriti <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* solve_network: modularize solver_options to allow for incremental config changes
config: harmonize and apply yaml linting

* refactor configs: boil down to effective diff in tutorial yaml and scenarios yaml

* test: remove config no_progress

* ci: disable for windows

* helpers: use copy_default_files

* config: make config.default + config.tutorial match old config.tutorial

* solve_network: fix options assignment in prepare

* add build_heating_distribution

* Update licensing

* Set location to Earth

* Update cluster pop

* Fix typos

* Bugfix to skip h2 pipelines with missing buses

* Bugfix h2_network loc

* reintroduce config.tutorial.yaml as basis (possibly revert this commit later to reenable config.default)

* doc: update testing documentation [skip ci]

* add comments to makefile

* Update myopic test

* Bugfix none location in build_industrial_database

* Revise run_test myopic name

* revert transpose

* delete plot_network_eur.py

* minor bug in build_base_energy_totals

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update build_base_industry_totals.py

* adapt build_base_energy and build_base_industry to params

* add missing param in snakefile

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Revise myopic test file

* fix param in snakefile

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* reset config.pypsa-earth.yaml

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update Snakefile

* handle exception to avoid reference before assignment error

* Add different scenario name for subworkflow

* Fix myopic test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update config.test_myopic.yaml to include new parameter

* replace snakemake subworkflow by submodule; fix path relations

* fill NaN without international bunkers

* remove factor for gas emissions from residential and services sector

* account for multi-country-cases

* snakefile: replace rdir by resdir for compatibility with pypsa-earth

* Update build_base_industry_totals.py

* Update Snakefile

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update build_base_industry_totals.py

* modified config to fix nan objective value

* print objective value to terminal

* fix for urls not working

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* added license for datasets

* Update README.md

* Update README.md

* Final README adaptations before v0.2 release (pypsa-meets-earth#364)

* Adding the updated network representation figure

* Delete docs/SCPE_v0.2.pdf

* Include new network representaion figure as png

* Update README.md

* Embed the new network figure in the readme

* Delete outdated incomplete network configurations

* updated pypsa-earth submodule to v0.4.0

* adjust modular solver options

* resolve global paths

* workflow: use global root directory to avoid recursive upwards chdir

* update pypsa-earth commit

* revert changes to config.default and config.tutorial priority

* udpate pypsa-earth

* consolidate CI yaml

* yamllint: align formatting and config

* Snakefile: fix SDIR and RESDIR path ending

* ci.yaml include git submodule

* fix duplicated "/"

* remove config.pypsa-earth.yaml in favour of actual pypsa-earth default config

* remove pypsa-earth.config from copy_config

* ci: use one core to better track log

* test: use more core

* make yamllint compatible

* build_renewable_profiles: use local client in order to suppress verbose dask output

* update submodule

* config: consolidate clustering key
snakefile: fix cost retrieval

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* solve_networks: fix network reference for sector coupled case

* address review comments

* config.myopic.yaml: revert country order again

* Update README.md

Co-authored-by: Davide Fioriti <[email protected]>

* address davides comments (first round)

* consolidate current working directory; considate helpers scripts

* fix reference to helpers script; fix fiona version

* env: update ppm version

* build_industrial_database: make retrieval robust against restrictive permissions

* follow up: consolidate restrictive url retrievals

* helpers: reinsert gadm functions from pypsa-earth-sec due to discrepancies (solve those later)

* env: temporarily install new earth osm version from pypi

* ci: restrict ipopt for windows

* env: follow up

* ci: roll back and disable windows

* _helpers: try make content_retrieval more robust

* Update README.md

Co-authored-by: Ekaterina <[email protected]>

* config: bump version; add `allow_scenario_failure` flag; remove `base` cutout comment

* Snakefile: make cutout path consistent for sector-coupled version
config.default: make hydro extedable again

* helpers: fix numpy random usage

* properly remove submodule

* add version tag to all configs

* env: update powerplantmatching

* add missing config version tag

* update powerplantmatching

* follow up: move ppm installation to pip while conda is not out

* readme: add description on running previous models

* helpers: add HTTPError as allowed exception

* config: remove lifetime from config.default

* harmonize cost calculation in sector coupled model

* bump version tag in readme [skip ci]

* update README to account for Hazem's comments

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: davide-f <[email protected]>
Co-authored-by: Hazem-IEG <[email protected]>
Co-authored-by: Davide Fioriti <[email protected]>
Co-authored-by: Hazem <[email protected]>
Co-authored-by: Eddy Jalbout <[email protected]>
Co-authored-by: finozzifa <[email protected]>
Co-authored-by: Anton Achhammer <[email protected]>
Co-authored-by: Anton Achhammer <[email protected]>
Co-authored-by: Fabrizio Finozzi <[email protected]>
Co-authored-by: Eddy-JV <[email protected]>
Co-authored-by: energyLS <[email protected]>
Co-authored-by: cpschau <[email protected]>
Co-authored-by: Emmanuel Bolarinwa <[email protected]>
Co-authored-by: Ekaterina <[email protected]>

* code: re-add copy_default_files

* code:fix imports

* code:fix arguments in build_osm_network

* code:geo_crs value as explicit argument

* code:remove unnecessary arguments

* code: modify the demand in scripts/build_base_energy_totals.py

* code: modify the demand in scripts/build_base_energy_totals.py - 2

* code: modify the demand in scripts/build_base_energy_totals.py - 3

* code: modify the demand in scripts/build_base_industry_totals.py

* code: modify the demand in scripts/build_base_industry_totals.py and scripts/build_base_energy_totals.py

* code:provide missing arguments in locate_bus

* code: add missing arguments in prepare_sector_network

* code: remove tuple

* code: remove tuple

* code:remove environment.mac and update test_build_powerplants

---------

Co-authored-by: Davide Fioriti <[email protected]>
Co-authored-by: Fabian Hofmann <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: davide-f <[email protected]>
Co-authored-by: Hazem-IEG <[email protected]>
Co-authored-by: Hazem <[email protected]>
Co-authored-by: Eddy Jalbout <[email protected]>
Co-authored-by: Anton Achhammer <[email protected]>
Co-authored-by: Anton Achhammer <[email protected]>
Co-authored-by: Eddy-JV <[email protected]>
Co-authored-by: energyLS <[email protected]>
Co-authored-by: cpschau <[email protected]>
Co-authored-by: Emmanuel Bolarinwa <[email protected]>
Co-authored-by: Ekaterina <[email protected]>
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.

I understand the previous PR was kept in Draft mode, opened to keep an eye on the working process (as stated in the text), with no request to review nor changes into review mode.
As such, no review was originally intended because of both points. However, we still decided to still give a revision of the PR with priority

I understand this PR combines:

  1. pytest: very interesting feature that we love to have. I quickly scanned them and a large part is likely well done, some tests may not be needed but are already available and that may not harm. I am aware you also reviewed them with Fabian too.
  2. revision of pathlib implementation: great feature as well to have
  3. architectural changes: such as the heavy revision of build_shapes and powerplantmatching
  4. lots of minor revision to include and improve the code

Features 1&2 are great and nearly ready to improve.

Feature 4 is also greatly appreciated; given the quantity hard to summarize them but by scanning them some are interesting but others may need some attention. Some comments and TODOs are dropped without explanation and they may be instead useful for developers. Naming conventions are changed sometimes improving the code, but that's not always the case.

Feature 3 is instead very delicate:

  1. Personally, I would not recomment moving the gadm stuff of build_shapes script into other scripts. The reason is that we want helpers to be light and general features, population filling is quite a specific task of build_shape and it would be better placed there. Moreover, the -sec model had a different standard use of gadm and the change should verify limited modelling differences. I believe and trust the approach but good to have that openly.
  2. There are heavy changes into build_powerplants that are not clear; it would be better to have a justification on why that is recommended.

It would be great to align on how to best proceed.

Having at least 2/3 PRs on specific tasks would be the standard approach. For example:

  1. for pathlib
  2. for pytest
  3. other improvements

Other approaches, such as reviewing this can be discussed also according to needs. It would take a larger toll but can be discussed.
It is of importance to solve this pending topic

@davide-f
Copy link
Member

davide-f commented Jan 4, 2025

Hello :D
As we are back of holidays, I'm writing the todos we agreed before going on holiday during last meeting with @FabianHofmann and @ekatef .

In the coming period, we aim to investigate the contributions and test them to merge them.
Contributions can be divided in 4 types:

  1. pytest contributions [basically straightforward to merge]
  2. pathlib implementation [nearly staightforward to merge, but little check may be helpful as it changes a lot of stuff]
  3. architectural changes (e.g. GADM revision and powerplants implementat), which need attention to ensure functionalities and reproducibility of results
  4. minor revisions that are generally fine

We investigate the best approach to merge the contributions:
a) review the PR as is
b) disentable the PR into simpler pieces easier to review and test

b) is generally preferrable if overhead is limited.

Plan forward:

  1. as the pytest implementations are generally stand-alone, creating a PR with them would be a good way forward. Fabian proposed to investigate this
  2. Katia and Davide will investigate the overlap of the other commits, aiming to define how to merge the other contributions, according to strategy a or b

Feel free to comment and revise :)

@davide-f
Copy link
Member

davide-f commented Jan 4, 2025

At first check, the major architectural changes (GADM and changes in powerplants) are added in this two contributions that are squash merges:

Therefore, the testing of the contributions separately should not add much overhead [in terms of conflicts while cherry-picking]

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.

5 participants