-
Notifications
You must be signed in to change notification settings - Fork 2
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
Os to pathlib and helpers.py method from pypsa-earth-sec #2
Conversation
…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
…a-earth into os_to_pathlib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments and questions
test/test_prepare_network.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pz-max
thanks for your comment. In order to execute this unit test I need to modify the way the _helpers.py methods are invoked in the other files contained in the scripts folder.
Currently we have from _helpers import ....
I think we need to have from scripts._helpers import ....
I will attempt this change in another pull request.
return df | ||
|
||
|
||
def download_gadm(country_code, update=False, out_logging=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Is this not already in PyPSA-Earth in build shapes? Why having it twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pz-max thanks for noticing it. Actually the two methods are kind of different in what they do even though they look similar and are called the same way.
I think that a dedicated PR is needed to fix it.
For the moment I will flag it as a TODO and change the same of the method in _helpers.py
return gadm_input_file_gpkg, gadm_filename | ||
|
||
|
||
def get_gadm_layer(country_list, layer_id, update=False, outlogging=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before? (It's in build shapes?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pz-max , ja same comment as before. I would like to have a dedicated pull request for address this "duplication". Most likely I need to properly unit test both version in order to merge them accordingly.
Hi all, as an extra check I have compared the values of the objective function we got from CI-linux/ ubuntu-latest for this pull request and ones obtained for CI-linux/ ubuntu-latest which corresponds to the last CI/CD exeuction for pypsa-meets-earth/pypsa-earth. In the attached files, I will refer to:
obj_func_new_values.txt As you can see, the percentage differences between the two executions are below 1%. |
…a-earth into os_to_pathlib
Changes proposed in this Pull Request
In this pull request I have:
os to pathlib
The os module has been replaced with the pathlib module. There are still a few os-module-calls in build_natura_raster.py (this is because there is no pathlib alternative for os.walk) and in the _helpers.py
The migration has been carried out together with the abstraction of the path methods. Such methods are available in the _helpers.py script. In fact, whenever I see lines of code that repeat themselfs several times, I try to create methods that implement such lines of code and that can be separately unit-tested. In this was I can ensure consistency and make future changes easier. Instead of tracing throughout the code each invocation of (say) os.path.basename(), I just need to modify once the method where such line is implemented. I believe that in this way, code reviews, code merges and further code expansions will be easier, as we can rely on soundproof methods that we have unit-tested.
Addition of the methods from the helpers.py script of pypsa-earth-sec
I did not add the following methods as they are not used in pypsa-earth-sec
Unit tests
I have added dedicated unit tests for the path methods and the methods coming from pypsa-earth-sec.
I have also added the execution of such unit tests to ci-linux.yaml, ci-mac.yaml, ci-windows.yaml, making the unit test execution part of the CI/CD.
Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.