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

Documentation improvement in the configuration section_v2 #694

Merged
merged 69 commits into from
May 6, 2023

Conversation

asolavi
Copy link
Contributor

@asolavi asolavi commented Apr 20, 2023

Closes #609.

This pull request aims to improve the documentation of the Configuration section. It addresses points 3, 4 and 5 of #609. Thanks @ekatef for the feedback. It is a fresh restart on PR #612, since it has been a while (and Albert changed GitHub account).

Changes proposed in this Pull Request

  1. The picture in the lines section includes too many things, the “lines” only appear at the very end.

  2. Create subsections in the electricity section, as in the renewables section.

  3. The section load from the documentation doesn’t seem to refer to anything on the config file. Shall we remove this?

  4. Add a short description in each subsection, as it is done in run.

  5. General consistency check accross the configuaration section.

Checklist

  • I consent to the release of this PR's code under the GPLv3 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 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.

@@ -3,12 +3,26 @@
# SPDX-License-Identifier: CC0-1.0

version: 1.0.0

Copy link
Member

Choose a reason for hiding this comment

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

Is this additional string added with some purpose?

My feeling is that tutorial flag and version are close enough by their meaning. Apart of that, it would be nice to keep the config as compact as we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ekatef, it was introduced for consistency, as every "section" of the config file is separated by a space. I agree it is pedantic in this case, so I will revert it back.

@@ -3,21 +3,13 @@
# SPDX-License-Identifier: CC0-1.0

version: 1.0.0

Copy link
Member

Choose a reason for hiding this comment

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

Which purpose does have this additional empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ekatef, same as in the config.default.yaml. Just me being a bit pedantic hehe. I will remove it as in config.default.yaml. Thanks for your feedback :)

@ekatef
Copy link
Member

ekatef commented Apr 24, 2023

Hey @asolavi! That is awesome to have you back ;)
I really like the changes you introduced.

Have you tested the updated documentation locally?

@asolavi
Copy link
Contributor Author

asolavi commented Apr 25, 2023

Hi @ekatef,

many thanks for your detailed feedback :). While working on this PR, I found a couple of questions/discussion points. I summarise them below. Your comments on them would be greatly appreciated.

  1. The scenario section in the "Configuration" documentation lost an image. It says "An exemplary dependency graph (starting from the simplification rules) then looks like this:" and then the text "img/scenarios.png" appears instead of the desired image. The problem seems to be that the img directory does no longer have the scenarios.png file. Do you have that image elsewhere, so that I could quickly add it? I assume it is a DAG of the workflow starting from simplification rules.

  2. What does the augmented line connection do? I found conflicting descriptions of the "connectivity_upgrade" in the config.default.yaml and what it is claimed to be used, namely NetworkX algorithms.
    a. config.default.yaml claims "connectivity_upgrade" is the minimum line connections per node.
    b. NetworkX states in the documentation that obtains a "k-edge-connected graph", where k is specified in the connectivity_upgrage parameter.

@asolavi
Copy link
Contributor Author

asolavi commented Apr 25, 2023

Hey @asolavi! That is awesome to have you back ;) I really like the changes you introduced.

Have you tested the updated documentation locally?

Thanks @ekatef, I'm very happy to be back, and hopefully can finish this PR sometime this week. All the sections that have been commited have been tested locally using make :). I'll keep doing that before commiting.

@ekatef
Copy link
Member

ekatef commented Apr 27, 2023

Hi @ekatef,

many thanks for your detailed feedback :). While working on this PR, I found a couple of questions/discussion points. I summarise them below. Your comments on them would be greatly appreciated.

Hey @asolavi! My pleasure and sorry for a delay with an answer. Chained deadlines are not always easy to deal with :) Answering point-by-point:

  1. The scenario section in the "Configuration" documentation lost an image. It says "An exemplary dependency graph (starting from the simplification rules) then looks like this:" and then the text "img/scenarios.png" appears instead of the desired image. The problem seems to be that the img directory does no longer have the scenarios.png file. Do you have that image elsewhere, so that I could quickly add it? I assume it is a DAG of the workflow starting from simplification rules.

Agree, "scenarios.png" seems to disappear... There is "img/workflow_introduction.png", but that is not exactly what is meant in Configuration. My feeling is that the most straightforward way would be simply to generate a new picture as "scenarios.png" and add it as a PR to documentation repo. If you would like to add an issue on that, it would be very helpful

  1. What does the augmented line connection do? I found conflicting descriptions of the "connectivity_upgrade" in the config.default.yaml and what it is claimed to be used, namely NetworkX algorithms.
    a. config.default.yaml claims "connectivity_upgrade" is the minimum line connections per node.
    b. NetworkX states in the documentation that obtains a "k-edge-connected graph", where k is specified in the connectivity_upgrage parameter.

My feeling is that the most comprehecsive description on augmentation procedure is contained in PyPSA-Earth paper: "A k-edge augmentation algorithm that guarantees every node has a modifiable number of connections to other nodes. Only if nodes do not already fulfil the connectivity condition, the algorithm will create new lines to the nearest neighbour by a minimum spanning tree. The new ‘augmented’ lines can be set to an insignificant size (e.g. 1 MW) to create new options for line expansion in the investment optimization"

Regarding connectivity_upgrage, that is k_edge_option in the code which is used in a call of k_edge_augmentation() which supplements the network graph with connections needed to make each node having at least the requested number of connections:

# apply k_edge_augmentation weighted by length of complement edges
# pick shortest lines per bus to fill k_edge condition (=degree of connectivity)
k_edge = k_edge_option
augmentation = k_edge_augmentation(
G, k_edge, avail=complement_edges[["bus0", "bus1", "length"]].values
)

I agree that currently the same property has too many different names which can be misleading. Feel free please to suggest a more clear way to explain it

@asolavi asolavi marked this pull request as ready for review May 3, 2023 10:43
@ekatef
Copy link
Member

ekatef commented May 4, 2023

Hi @asolavi, regarding the questions on renewables configuration:

What does the field renewable"technology"\ extendable do? Here "tecnology" can replace any of onwind, offwind-ac, offwind-dc, solar. Seems to be used for hydro only in the scripts.

extendable is used is add_electricity to flag technologies for which zero-capacity generators shall be included for further optimisation. Actually, that is quite important. A very good catch, as always ;)

Am I correct in assuming that offshore-ac has a maximum distance beyond which it cannot be placed, whereas onshore-ac has a minimum distance under which it cannot be placed? In other words, offshore-ac is for windfarms close to shore, and onshore-dc for windfarms far away from shore.

You are absolutely correct :)

What do the fields renewable\hydro\resource\method, renewable\hydro\resource\hydrobasin, and renewable\hydro\resource\flowspeed do? What are their options other than the ones already in the config file?

hydrobasin is used to define a polygon to calculate a surface integral when making runoff calibration, resource\method determines an Atlite method to calculate a renewable potential (wind, solar, hydro); there is also another method for hydro normalization\method, which determines a data source to match agains when making runoff calibration

As for hydro\resource\flowspeed, not sure if it's currently in use. @davide-f can give the most accurate information on it

@ekatef
Copy link
Member

ekatef commented May 4, 2023

I believe this PR is now ready for review :).

Super!!! :)

Just to double check, a few sections that I have removed because I found they were not in the config.default.yaml file:

  1. load : Am I correct in assuming that it got changed by the load_option settings? This was done in commit b9183f7.

Agree, load.csv looks kind-of outdated

  1. costs: The capital_cost option is no longer there. Is this expected? See commit 9022ad6.

It looks like capital_cost was replaced by investment

  1. clustering: Got renamed cluster_options. I just kept the second one, as they were copies of each other pretty much. See 24db260.

Absolutely correct :)

@ekatef
Copy link
Member

ekatef commented May 4, 2023

@asolavi, absolutely agree that it's perfect time for review :)
Amazing work!

@asolavi
Copy link
Contributor Author

asolavi commented May 5, 2023

Thank you very much @ekatef for the detailed information :). All points below have been addressed.

Hi @asolavi, regarding the questions on renewables configuration:

What does the field renewable"technology"\ extendable do? Here "technology" can replace any of onwind, offwind-ac, offwind-dc, solar. Seems to be used for hydro only in the scripts.

extendable is used is add_electricity to flag technologies for which zero-capacity generators shall be included for further optimisation. Actually, that is quite important. A very good catch, as always ;)

Added the extendable row in the corresponding tables.

Am I correct in assuming that offshore-ac has a maximum distance beyond which it cannot be placed, whereas onshore-ac has a minimum distance under which it cannot be placed? In other words, offshore-ac is for windfarms close to shore, and onshore-dc for windfarms far away from shore.

You are absolutely correct :)

OK, thanks. No action needed here as it was written already assuming it in the corresponding commit.

What do the fields renewable\hydro\resource\method, renewable\hydro\resource\hydrobasin, and renewable\hydro\resource\flowspeed do? What are their options other than the ones already in the config file?

hydrobasin is used to define a polygon to calculate a surface integral when making runoff calibration, resource\method determines an Atlite method to calculate a renewable potential (wind, solar, hydro); there is also another method for hydro normalization\method, which determines a data source to match agains when making runoff calibration

As for hydro\resource\flowspeed, not sure if it's currently in use. @davide-f can give the most accurate information on it

Great, the rows for hydrobasin and method now have a description. Didn't add anything for the flowspeed, will add it once the information becomes available.

@asolavi
Copy link
Contributor Author

asolavi commented May 5, 2023

Thanks again for your help, @ekatef. If this looks reasonable, all good. These were removed already in the corresponding commits, so no further action required :).

I believe this PR is now ready for review :).

Super!!! :)

Just to double check, a few sections that I have removed because I found they were not in the config.default.yaml file:

  1. load : Am I correct in assuming that it got changed by the load_option settings? This was done in commit b9183f7.

Agree, load.csv looks kind-of outdated

  1. costs: The capital_cost option is no longer there. Is this expected? See commit 9022ad6.

It looks like capital_cost was replaced by investment

  1. clustering: Got renamed cluster_options. I just kept the second one, as they were copies of each other pretty much. See 24db260.

Absolutely correct :)

@ekatef
Copy link
Member

ekatef commented May 5, 2023

@asolavi, I think it looks great!
Fantastic work, and thank you so much for that ❤️

Have just recognised that adding the config tables, you have also addressed a big (and the most scary) part of #573

@pz-max I think this PR is perfectly ready for the review. Would you mind to look into it?

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.

Great @asolavi. This is a massive contribution. I will merge some little changes in a moment.

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. @ekatef feel free to merge. (sorry for not running pre-commit before :/)

My changes:

  • theme change according pypsa-eur
  • reduce warnings from 250 warning to 53 warnings when compiling the HTML with make html (e.g. using width instead of :scale:, add/remove requirements, fix some links)

@ekatef ekatef merged commit ac5ed2d into pypsa-meets-earth:main May 6, 2023
@ekatef
Copy link
Member

ekatef commented May 6, 2023

@pz-max thanks a lot for the great review and fixing the docs infrastructure. Agree the the new theme looks charming in fact 😍

@asolavi huge thanks for the fantastic contribution! 🥇 🎉 🎉
I think that is a big step forward in improving model usability

@asolavi
Copy link
Contributor Author

asolavi commented May 7, 2023

Thank you @ekatef for your crucial help throughout this PR.

Thank you @pz-max for the review and the many final editions.

Maybe related to the PR in PyPSA-Europe, I've noticed that building the docs now shows this error:

Extension error:
Could not import extension myst_parser (exception: No module named 'myst_parser')

It doesn't seem to affect the creation of the configuration page of the docs, but suspiciously now building the documentation takes much less time. I've tried that in a freshly installed environment pypsa-earth-docs, and still same error.

@asolavi
Copy link
Contributor Author

asolavi commented May 7, 2023

Thank you @ekatef for your crucial help throughout this PR.

Thank you @pz-max for the review and the many final editions.

Maybe related to the PR in PyPSA-Europe, I've noticed that building the docs now shows this error:

Extension error: Could not import extension myst_parser (exception: No module named 'myst_parser')

It doesn't seem to affect the creation of the configuration page of the docs, but suspiciously now building the documentation takes much less time. I've tried that in a freshly installed environment pypsa-earth-docs, and still same error.

PS: Just seen that @pz-max has a commit add myst_parser to conf.py in PR 658 from PyPSA-Eur which I suspect should fix this.

@pz-max
Copy link
Member

pz-max commented May 7, 2023

@asolavi, well spotted.
I think we can remove pypsa-earth/envs/environment.docs.yaml. I used pip install -r doc/requirements.txtwhere myst-parser was integrated and which is actually used by our .readthedocs.yaml. It's also quite fast to install than the docs environment.

Do you like to add a PR @asolavi removing the environment.docs.yaml, adjust how_to_doc documentation etc accordingly? (In VS code I am using the CTRL+shift+F to find names across scripts

@ekatef
Copy link
Member

ekatef commented May 7, 2023

@asolavi that is a pleasure to collaborate with you. Happy to continue 😉

Absolutely agree with @pz-max that an update would be very helpful on how to build a doc environment. I also needed a couple of attempts yesterday to get the updated version work :)

Currently, doc instructions are included into how_to_doc file which is embedded into tutorial section with .. include:: ./how_to_docs.rst

@asolavi
Copy link
Contributor Author

asolavi commented May 8, 2023

I see, thank you @pz-max and @ekatef. A freshly-looking documentation is created on my local machine too :).

I've started the changes discussed in PR #720. Should be quick.

@asolavi asolavi deleted the doc_config branch May 8, 2023 13:52
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.

Update on documentation - configuration section
3 participants