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 improvements in the Configuration section #612

Closed
wants to merge 12 commits into from
Closed

Documentation improvements in the Configuration section #612

wants to merge 12 commits into from

Conversation

asv365
Copy link

@asv365 asv365 commented Feb 20, 2023

Closes

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.

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 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.

@asv365
Copy link
Author

asv365 commented Feb 20, 2023

Point 1 is caused by the appearance of the text "lines:" before the lines section header. The lines section header is where the image should start to be displayed.

For context, the images are created using the instruction:

.. literalinclude:: ../config.default.yaml
:language: yaml
:start-at: lines:
:end-before: links:

Since the text ''lines:'' appears before in the config.default.yaml file (namely in the instruction split_overpassing_lines: true), the image displayed is from that first appearance of "lines:" until the first appearance of the word "links:".

Haven't found a good fix yet, but to illustrate the problem the first commit changes "lines:" for "PV:", which is an improvement.

@ekatef
Copy link
Member

ekatef commented Feb 23, 2023

@asv365 A very good point regarding catching text from a wrong fragment. Thanks a lot for investigating it!

Probably, Sphinx docs on literalinclude could be helpful to resolve this, eventually combined with other Shpinx magic. The most straightforward way seems to be use tag comments. But I suspect there might be a more elegant solution, as well.

@asv365
Copy link
Author

asv365 commented Feb 24, 2023

Thanks @ekatef, the suggestion on comments can be a good workaround if we can't find how to do it properly with Sphinx. If Sphinx proves tricky, we could add a header to every section of the config file, to be consistent.

@asv365
Copy link
Author

asv365 commented Feb 27, 2023

On point 3., the section "load" has been renamed "operational_reserve", and has been placed as a subsection of electricity. There is also the section "load_options", which doesn't exist in the documentation. This will be addressed as point 5, as part of the general consistency check.

@asv365
Copy link
Author

asv365 commented Feb 27, 2023

@ekatef found a solution to the point 1 we were discussing earlier without the need to use comments in the config file.

Literalinclude allows the command start-after as well as start-at. start-after ignores the line you are pointing to, and starts displaying in the next one. Since the previous line is PV:, which is unique, it works for this case.

@ekatef
Copy link
Member

ekatef commented Feb 27, 2023

@ekatef found a solution to the point 1 we were discussing earlier without the need to use comments in the config file.

Literalinclude allows the command start-after as well as start-at. start-after ignores the line you are pointing to, and starts displaying in the next one. Since the previous line is PV:, which is unique, it works for this case.

@asv365 a very nice trick which looks absolutely in spirit of Sphynx! :)

Probably, there is a little risk to catch an additional section in case it would be inserted between estimate_renewable_capacities and lines... Could it make sense to add a comment to configuration.rst as a reminder on why do we refer exactly PV?

Although, the work you are doing is still a great improvement for clarity of the docs 🥇

@asv365
Copy link
Author

asv365 commented Feb 28, 2023

Good point @ekatef. I'll add that comment in configuration.rst

@asv365
Copy link
Author

asv365 commented Feb 28, 2023

@ekatef The top-level configuration section (which, according to the table in the docs includes "version", "tutorial", "logging", "countries", and "enable") has other sections inbetween. For example, "run" and "scenarios" are between "logging" and "countries" in the config.default.yaml.

Do we want to bring all the items of the top-level configuration together at the top of the config.default.yaml and config.tutorial.yaml? Not doing that forces literalinclude to display using line numbers, which was creating trouble earlier. See, e.g., commit d71a3a5 above.

Another option would be to include those intermediate sections as part of the top-level configuration. Let me know what you think :).

@ekatef
Copy link
Member

ekatef commented Mar 1, 2023

@asv365, thanks for rising this point! Our current top-level configuration is in fact rendered in not optimal way...

Completely agree that it might be worth to revise underlying assumptions for making the result more robust and the docs more convenient. I feel that it is a very good point for discussion during developers meeting :) Would you mind to introduce this topic tomorrow?

@asv365
Copy link
Author

asv365 commented Mar 1, 2023

Thank you @ekatef. I'll raise it in tomorrow's meeting :)

@asv365
Copy link
Author

asv365 commented Mar 2, 2023

In the pypsa-earth weekly meeting on 02/03/2023, it was decided that:

  1. In literalinclude, preference of "start-at" like command instead of hard-coded line numbers.
  2. The top-level configuration section should include the sections that are already there. Reshuffling in config.default.yaml allowed, but keep to the bare minimum and report it in the PR. Another option is to include several "literalincludes".
  3. Make the Value column as short as possible, and very symbolic.

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.

pre-commit needs fixes as it could not automatically get fixed.
How far are we from merging this PR @asv365 ?

@@ -25,7 +25,7 @@ Top-level configuration

.. literalinclude:: ../config.default.yaml
:language: yaml
:lines: 5-12,20,31-38
:lines: 5-12,24,31-38
Copy link
Member

Choose a reason for hiding this comment

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

I think we will change that by the start-at and end-at stuff?

Copy link
Author

Choose a reason for hiding this comment

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

yes @pz-max, that is the plan. Just did this to motivate the start-at and end-at approach. Will work through this soon I hope, a lot of work has popped up recently.

@ekatef
Copy link
Member

ekatef commented Mar 27, 2023

Hey @asv365! Very nice work so far :)
Could you please give an update on the PR status? Don't hesitate to let us know if you need any additional inputs. Would be happy to support you if needed

@asv365
Copy link
Author

asv365 commented Mar 29, 2023

Hi @ekatef, everything is good, thanks for asking and for offering help. I don't think there are bottlenecks so far.

The reason for being so slow are job applications. Will come back to this in the next week or the one after hopefully. The dust is settling now. Apologies for the delay.

@ekatef
Copy link
Member

ekatef commented Mar 29, 2023

@asv365, no worries! Good luck with your applications and come back once you'd have more time ;)

@asolavi
Copy link
Contributor

asolavi commented Apr 20, 2023

This pull request is being addressed in PR #694 because a fresh restart was useful.

@davide-f
Copy link
Member

davide-f commented May 2, 2023

Hello :)
I'm closing this PR as it is continued into the other PR.
Thanks for the contribution!

@davide-f davide-f closed this May 2, 2023
@asolavi
Copy link
Contributor

asolavi commented May 3, 2023

Hello :) I'm closing this PR as it is continued into the other PR. Thanks for the contribution!

Thank you, yes!

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