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

Remove content tables and update tutorial files #872

Merged
merged 9 commits into from
Apr 30, 2024

Conversation

luseverin
Copy link
Collaborator

@luseverin luseverin commented Apr 12, 2024

This PR removes content tables from the CLIMADA tutorials and makes various fixes (typos and readability improvements).
Changes proposed in this PR:

  • Remove the table of contents on top of each tutorial file
  • Harmonize the headers of each tutorial file
  • Update guidance on header structure in developer guide
  • Removes dark font in climada_Entity_exposures.ipynb to improve readability
  • Fixes various typos in the tutorials

This PR fixes #862

PR Author Checklist

PR Reviewer Checklist

@luseverin
Copy link
Collaborator Author

luseverin commented Apr 12, 2024

Some additional remarks:

@spjuhel
Copy link
Collaborator

spjuhel commented Apr 15, 2024

Thanks for the cleanup!

There is a ParseError in climada_engine_CostBenefit.ipynb (see the attached screenshot). It appears when I open the tutorial file with VScode, but not online: https://github.com/CLIMADA-project/climada_python/blob/0b432a774bcbff2a56c5bfcf03712b6e990a0831/doc/tutorial/climada_engine_CostBenefit.ipynb . Let me know if you also have this error when you view the file with VScode.

@viggowat @chahank Ideas on this? Should we fix this within this PR?

I did not touch the "How is this tutorial structured" parts (e.g in hazard tutorial, and https://github.com/CLIMADA-project/climada_python/blob/main/doc/tutorial/climada_hazard_TropCyclone.ipynb), as I was not sure they should be considered as table of contents. However, as they also contain links, we might want to remove them. Let me know if they should be removed and I will take care of it.

I think it is also redundant with the ToC in the sidebar, and it really does not bring anything else, so I would remove it also.
@NicolasColombi @chahank what do you think?

@chahank
Copy link
Member

chahank commented Apr 16, 2024

Thanks for the cleanup!

There is a ParseError in climada_engine_CostBenefit.ipynb (see the attached screenshot). It appears when I open the tutorial file with VScode, but not online: https://github.com/CLIMADA-project/climada_python/blob/0b432a774bcbff2a56c5bfcf03712b6e990a0831/doc/tutorial/climada_engine_CostBenefit.ipynb . Let me know if you also have this error when you view the file with VScode.

@viggowat @chahank Ideas on this? Should we fix this within this PR?

I did not touch the "How is this tutorial structured" parts (e.g in hazard tutorial, and https://github.com/CLIMADA-project/climada_python/blob/main/doc/tutorial/climada_hazard_TropCyclone.ipynb), as I was not sure they should be considered as table of contents. However, as they also contain links, we might want to remove them. Let me know if they should be removed and I will take care of it.

I think it is also redundant with the ToC in the sidebar, and it really does not bring anything else, so I would remove it also. @NicolasColombi @chahank what do you think?

I agree, I would remove the links for sure, and probably the entire section.

@chahank
Copy link
Member

chahank commented Apr 29, 2024

@luseverin : do you think you have the information to finish this PR?

@luseverin
Copy link
Collaborator Author

@luseverin : do you think you have the information to finish this PR?

Almost everything yes. Following Sam's and your suggestions I will remove all occurences of "How is this tutorial structured" subsections.

Then the only thing I am missing is what to do with this ParseError in climada_engine_CostBenefit.ipynb. I can try to have a look and fix it within this PR if you think it is worth it. Otherwise I can open another issue linked to it or just let it go..
In any case it would be nice if some of you could tell me if they also see this error or if this is just specific to my machine/version of vscode..

@chahank
Copy link
Member

chahank commented Apr 29, 2024

Excellent! As for the parsing error, I have not had the problem. But, if you find quickly a fix, sure include it. Otherwise, I would open a separate issue. Maybe @spjuhel can say whether he also has the same error?

@luseverin
Copy link
Collaborator Author

Ok so I have removed the remaining "how is this tutorial structured" parts, and fixed the ParseError (following https://tex.stackexchange.com/questions/661333/big-fails-with-invalid-delimiter-type-ordgroup-in-vscode-ipynb-md-cell). I just changed the \big{(} to \big(. Normally this should not create further issues..

As far as I'm concerned, we can merge and close this PR.

@chahank
Copy link
Member

chahank commented Apr 29, 2024

Excellent, thanks! From my point of the view, this can be merged. If you find time, I think on climada petals the only tutorial that is not following the convention is https://climada-petals.readthedocs.io/en/stable/tutorial/climada_exposures_openstreetmap.html (the sections are not properly assigned and thus the table of content is empty).

You may then also close #862 .

@luseverin
Copy link
Collaborator Author

Ah yes, I did not think of checking the tutorial on petals. So I removed the content table from the climada_exposures_openstreetmap.ipynb and from climada_hazard_RiverFlood.ipynb. The changes are on a branch also called feature/update_tuto_files on climada_petals.
I had a brief check at the other tutorials and I also noted that some cells are failing to run in the climada_hazard_entity_Crop.ipynb. Are you aware of this?

@chahank
Copy link
Member

chahank commented Apr 30, 2024

Thanks! Can you make a PR on climada petals and we continue the discussion there? Then it is a bit clearer, and you may merge this PR here.

@luseverin luseverin merged commit 07f7281 into develop Apr 30, 2024
11 of 12 checks passed
@emanuel-schmid emanuel-schmid deleted the feature/update_tuto_files branch May 8, 2024 07:29
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.

Links in tutorials within the doc do not work (the ones on the right side navigation bar are ok)
3 participants