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

Implement of documentation #61

Merged

Conversation

Margherita-Capitani
Copy link
Contributor

First proposal to improve the documentation. An explanation is added on the tutorial, and the basic configuration in the configuration file. The next step would be to explain how to run the code in the ‘non-tutorial’ case.
I am not sure if I have inserted the images correctly.

Margherita-Capitani and others added 23 commits January 3, 2025 14:18
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.

Amazing @Margherita-Capitani :D
I see this PR builds on top of the previous PR, so priority is merging the otherone.
I've added a couple of comments and great!
Out of curiosity, have you used ChatGPT for revising the documentation?

countries: ["NG"]


Please remember,before proceeding, that as has already been mentioned, in case you are running a simulation in tutorial mode you will only be able to select one nation from NG,BJ,BW or MA;
Copy link
Member

Choose a reason for hiding this comment

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

check the spacing.
Moreover, for improving the styling, I'd recommend to do:

pre-commit install
pre-commit run --all

The code will change the styling of files and some grammar things.
For example, here the spacing here. In the text here, we can use the full names Nigeria, Benin etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, yes sorry i always forget of the pre-commit! One question: but after i do pre-commit run --all it fix all of this type of error automatically?

Configure `coordinates`
--------------------------
Currently, the tool allows us to select the coverage area of interest by going to enter the coordinates of the vertices of the rectangular surface we want to be covered.
At the end of the config.yaml file you will find the microgrid list section where you can go to enter the coordinates.
Copy link
Member

Choose a reason for hiding this comment

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

mention the microgrig_list parameter using the ` character so that it will be highlighted in the text


Regarding topics in the enable section:

- Retrive_databundle and retrive_cost_data: After the initial run, it is a good idea to set retrive databundle and cost data to false to avoid re-downloading unnecessary data.
Copy link
Member

Choose a reason for hiding this comment

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

when referencing option names e.g. "retrieve_databundle", it would be great to surround them with the characters to improve the styiling, such asretrieve_databundle`

In in the configuration file you will also find the variable "year" this specifies the reference year for the local population data.
It is your choice whether or not to keep the data temporally concordant

specify the method of load calculation :
Copy link
Member

Choose a reason for hiding this comment

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

First letter uppercase and avoid the spacing at the end "calculation :" -> "calculation:"

Copy link
Member

Choose a reason for hiding this comment

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

We try to avoid adding binaries to the repo as they increase the size of the overall github repo.
There are special exception and this may be one, but this file seems a bit large.

A proposal is to remove this from github history and potentially load a smaller file as the procedure is complete.
It would be advisable to resize this file around 100kb or so [currently it is 1.4Mb]

Luckily, images have been added only recently and so the fix can be done by:
[DANGER: this is going to overwrite the github history and you may lose local changes as well as some implemented changes; it is recommended to create a copy of the folder first]
git reset 62b32af --hard

That line restores the github history to that commit.
After that commit, there are only 2 commits to be re-implemented:

  • 3822899
  • c78a7df [to do this one, you can do pre-commit run --all before pushing the changes of the previous commit and commit the changes that the pre-commit proposes]

Happy to support

Copy link
Member

Choose a reason for hiding this comment

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

Fantastic!

@Margherita-Capitani
Copy link
Contributor Author

Amazing @Margherita-Capitani :D I see this PR builds on top of the previous PR, so priority is merging the otherone. I've added a couple of comments and great! Out of curiosity, have you used ChatGPT for revising the documentation?

Perfect, yes I had noticed that I had made some mistakes in opening the PR, I replied to the comments in the other PR and it should be ready for the Merge :D
In this part not so much, i write the text by my self after reading the one of PyPSA-Earth documentation, i work on the one that i've made during the thesis trying to inprove them! in this part i use it most of all for understand how to add the images and for correct the language errors! :D

@davide-f davide-f marked this pull request as ready for review February 2, 2025 13:06
@davide-f davide-f merged commit e032702 into pypsa-meets-earth:main Feb 2, 2025
2 checks passed
@davide-f
Copy link
Member

davide-f commented Feb 2, 2025

Merged :D great contribution! :)
I know shankar has also ideas here and could be good to align and propose further improvements :)
Happy to align on discord and on the next tuesday meeting on the 11th :)

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.

2 participants