CLIMADA-project / climada_python

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

Integration of CalcDeltaImpact class of unsequa into develop #844

Closed simonameiler closed 9 months ago

simonameiler commented 10 months ago

Changes proposed in this PR:

PR Author Checklist

PR Reviewer Checklist

chahank commented 10 months ago

Can you please merge the develop branch into this branch? All tests are now failing, but I think it is due to a bug that is fixed on develop.

chahank commented 10 months ago

I have one larger general question: from the naming, and what the docstring says, I would expect the new module to compute the absolute differences. However, the current code seems to compute relative differences. What is the rational here? This should maybe be an option?

After these points are resolved, someone should still do a careful check of the actual methods. I briefly went over them, and it seems all fine. But details can be important, in particular also because the test is quite minimal.

simonameiler commented 10 months ago

Can you please merge the develop branch into this branch? All tests are now failing, but I think it is due to a bug that is fixed on develop.

I did. I don't think the bug is fixed on develop yet.

simonameiler commented 10 months ago

I have one larger general question: from the naming, and what the docstring says, I would expect the new module to compute the absolute differences. However, the current code seems to compute relative differences. What is the rational here? This should maybe be an option?

After these points are resolved, someone should still do a careful check of the actual methods. I briefly went over them, and it seems all fine. But details can be important, in particular also because the test is quite minimal.

I resolved all points to my best knowledge. I don't think there should be an option to calculate delta impacts for absolute values. Absolute delta impact calculations are not useful without knowledge of a reference state. I added a few lines in the docstrings reasoning for the relative delta impact calculation.

simonameiler commented 9 months ago

This looks good and useful! There's not much left to do:

  • Please resolve all linter issues (except too-many-arguments and too-many-local-variables). This should be rather simple
  • The module documentation is missing. Please add an appropriate section to doc/climada/climada.engine.unsequa.rst and double-check the rendered docs.
  • The tutorial section on the new class should probably start with an explanation what the class does (current paragraphs 2 and 3), and then mention that it works analogously to the other class (paragraph 1)
  • The table of contents in the tutorial was not updated, and the links don't work anyway. Maybe just remove it for now?

I addressed all your points. In my eyes, it's good to go.