CLIMADA-project / climada_python

Python (3.8+) version of CLIMADA
GNU General Public License v3.0
305 stars 119 forks source link

Remove content tables and update tutorial files #872

Closed luseverin closed 5 months ago

luseverin commented 5 months ago

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

This PR fixes #862

PR Author Checklist

PR Reviewer Checklist

luseverin commented 5 months ago

Some additional remarks:

spjuhel commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

luseverin commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.