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

doc: update config section #685

Merged
merged 23 commits into from
Jul 17, 2023
Merged

doc: update config section #685

merged 23 commits into from
Jul 17, 2023

Conversation

FabianHofmann
Copy link
Contributor

Update the documentation's config section. Pull out config comments to documentation.

@virio-andreyana virio-andreyana marked this pull request as ready for review July 5, 2023 00:05
@FabianHofmann
Copy link
Contributor Author

@fneum @lisazeyen could one of you also have a look? In particular, at the documentation of the sector section at https://pypsa-eur--685.org.readthedocs.build/en/685/configuration.html#sector

Copy link
Member

@fneum fneum left a comment

Choose a reason for hiding this comment

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

Excellent! Super helpful!

I have made small adjustments here and there.

One thing to be careful about: When the documentation says percentage, I would put e.g. 20.7 for 20.7%, but in most cases the input requires 0.207. Maybe one could clarify this.

My other comment is stylistic. Could you check if it makes sense to adjust the column widths of the tables and give more width to the description?

doc/configtables/.~lock.sector.csv# Outdated Show resolved Hide resolved
doc/configuration.rst Outdated Show resolved Hide resolved
doc/configuration.rst Show resolved Hide resolved
doc/configtables/electricity.csv Outdated Show resolved Hide resolved
doc/configtables/electricity.csv Outdated Show resolved Hide resolved
doc/configtables/sector.csv Outdated Show resolved Hide resolved
doc/configtables/sector.csv Outdated Show resolved Hide resolved
doc/configtables/industry.csv Outdated Show resolved Hide resolved
doc/configtables/industry.csv Outdated Show resolved Hide resolved
doc/configtables/industry.csv Outdated Show resolved Hide resolved
@FabianHofmann
Copy link
Contributor Author

Hey @virio-andreyana, there are still some unresolved issues. Would you look into them?

@virio-andreyana
Copy link
Collaborator

Okay, so the simple suggestion have been added. Now, only a few problem is left.

One thing to be careful about: When the documentation says percentage, I would put e.g. 20.7 for 20.7%, but in most cases the input requires 0.207. Maybe one could clarify this.

Hmm, I believe that the reader is smart enough to know how the meaning of the percentage works. So long as they see the config default value.

My other comment is stylistic. Could you check if it makes sense to adjust the column widths of the tables and give more width to the description?

I could try that, maybe by shortening the column for the attribute name, and values

@FabianHofmann
Copy link
Contributor Author

thanks @virio-andreyana, perhaps we use share instead of percentage?

@FabianHofmann
Copy link
Contributor Author

@fneum would you be fine with merging?

@fneum
Copy link
Member

fneum commented Jul 14, 2023

The CSVs now have quite a few empty lines. Might cause problems.

@fneum
Copy link
Member

fneum commented Jul 14, 2023

Otherwise, yes!

@FabianHofmann
Copy link
Contributor Author

The CSVs now have quite a few empty lines. Might cause problems.

@fneum thanks for looking over
@virio-andreyana this seems to be the last remaining issue

@virio-andreyana
Copy link
Collaborator

The CSVs now have quite a few empty lines. Might cause problems.

Actually this is a dilemma. So what I wanted to achieve is to have all of the table to have the same spacing. But since some attribute have letters longer than 22 letters, the table shifts to the right. Making the room for the description smaller.

This is the case especially for sector.csv and industry.csv

@FabianHofmann
Copy link
Contributor Author

The CSVs now have quite a few empty lines. Might cause problems.

Actually this is a dilemma. So what I wanted to achieve is to have all of the table to have the same spacing. But since some attribute have letters longer than 22 letters, the table shifts to the right. Making the room for the description smaller.

This is the case especially for sector.csv and industry.csv

Sound reasonable. Let's keep the extra lines, then. In the docs it looks good.

@FabianHofmann FabianHofmann enabled auto-merge July 17, 2023 15:37
@FabianHofmann FabianHofmann merged commit 9936953 into master Jul 17, 2023
@FabianHofmann FabianHofmann deleted the describe-config branch July 17, 2023 16:18
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.

3 participants