Skip to content

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:

@luseverin luseverin requested a review from spjuhel April 12, 2024 15:26
@spjuhel spjuhel requested a review from NicolasColombi April 15, 2024 16:19
@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
@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)

4 participants