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

Integration of CalcDeltaImpact class of unsequa into develop #844

Merged
merged 62 commits into from
Jan 31, 2024

Conversation

simonameiler
Copy link
Collaborator

@simonameiler simonameiler commented Jan 18, 2024

Changes proposed in this PR:

  • Integration of CalcDeltaImpact class of unsequa into develop
  • Update unsequa tutorial
  • Modification of plotting functionalities of the uncertainty output in unc_output to skip columns of the uncertainty output dataframe which are zeros or nan.
  • addition of a small function called save_divide in util which handles division by zero and NaN values in the numerator or denominator. This is needed for the relative impact change calculation (delta impact). Tests for the function are also added.

PR Author Checklist

PR Reviewer Checklist

Chahan Kropf and others added 30 commits November 3, 2022 11:41
* update __init__ in tc_tracks.py

* update tc_tracks.py

* update tc_tracks.py

* remove trailing white space

* update __init__ for hazard.tag

* und changes on tc_tracks

* engine.impact: proper hazard.tag initialization

* cosmetics

* set hazard_type de facto mandatory
(de jure still optonal)

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* Add type hints to Forecast.__init__ and update docstring

* Update climada/engine/forecast.py

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
Co-authored-by: Emanuel Schmid <51439563+emanuel-schmid@users.noreply.github.com>
* update __init__ in tc_tracks.py

* update tc_tracks.py

* update tc_tracks.py

* remove trailing white space

* Update usage of TCTracks.__init__

* Update TCTracks.__init__ docstring

Add information on `data` parameter.

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
* update __init__ entity.tag

* entity.tag.Tag.__init__: undo skip casting

Co-authored-by: Emanuel Schmid <51439563+emanuel-schmid@users.noreply.github.com>
…551)

* u_coord: _add_gdal_vsi_prefix

* read_raster_sample: gradient feature

* Fix tests for dist_to_coast_nasa

* u_coord._raster_gradient: lat direction first

* u_coord.read_raster_sample: support different interp methods for data and its gradient

* read_raster_sample_with_gradients

* u_coord: pylint unused variable

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* Assume fraction is one if None

* Allow fraction to be empty 

When fraction is empty, it is ignored in the impact calculation. The shape must still be n_events x n_exp

* Correct shape to argument

* Remove unecessary else

* Return None in get_fraction

* Set default return full fraction

* Update test TC for fraction None

* Remove test nozero fraction elements

* Make get_fraction private to _get_fraction

* Correct typo get_fraction -> _get_fraction in test

* Add note that fraction is optional in hazard tutorial

* Add note that probabilistic does not mean no storyline

* Fix equation rendering

* Fix linter issues: Single statement per line

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

* Set test to assert_array_equal for get_fraction

* Update hazard test file to hdf5 without fraction

* Update climada/engine/impact_calc.py

Note, the pylint message is then disabled for the whole function.

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

* Make pylint ignore warning for one line only

* Update loading of tc hazard file

* Update loading of test hazard file

* Fix test file path

* test file handling consolidation

* hazard.base: get_fraction has become private

Co-authored-by: Chahan Kropf <chahan.kropf@posteo.com>
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* Set all Hazard attributes via __init__

This change is backwards compatible as all defaults remain the same

* Add type hinting to Hazard.__init__

* Make fraction default size the same as intensity

In order to be compatible with the planed changes to the fraction attribute (if empty, it is ignored), the fraction should by default be derived from the shape of the intensity attribute.

* Update Hazard.__init__ signature and docstring

* Add kwargs for HazardTag
* Fix type hinting

* Adapt unit tests to new Hazard constructor

* Update tutorials to new Hazard constructor

* Add test for initializing Hazard without fraction

* undo re-plotting

Co-authored-by: Chahan Kropf <chahan.kropf@posteo.com>
Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* add type hints

* trop_cyclone: order imports

* trop_cyclone: remove trailing whitespaces

* trop_cyclone: np.array -> np.ndarray

* trop_cyclone: pass kwargs to Hazard base class

* TropCyclone: more type hinting

Co-authored-by: Thomas Vogt <thomas.vogt@pik-potsdam.de>
Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* Correction of tutorials: spelling and bugs

1) First correction: Change in cell 14 of tutorial 1_main_climada.jpynb
An extra comma "," was making an array behave like a tuple.

1) Second correction: Change in cell 2 of climada_engine_Forecast.jpynb
Spelling mistake: there was two times in a row the word "with" and two
times ##

* undo replotting

Co-authored-by: Nicolas Colombi <ncolombi@student.ethz.ch>
Co-authored-by: emanuel-schmid <schmide@ethz.ch>
* Add all ImpactFunc attributes to ImpactFunc.__init__

* Add type hints to ImpactFunc.__init__

* Update ImpactFunc classmethods to new init

* Update tests to new ImpactFunc.__init__

* Update docs on InputVar regarding ImpactFunc

* Update tutorials with new ImpactFunc.__init__

* Update ImpactFunc.__init__ docstring

* Update argument order of ImpactFunc.__init__

* Fix linter issue

* Update tests to new ImpactFunc.__init__

* Update tutorials with new ImpactFunc.__init__

* Enable passing impact funcs to ImpactFuncSet constructor

* Update code base to new ImpactFuncSet constructor

* Fix unit test of ImpactFuncSet

Remove initialization with set because it does not guarantee insertion
order.

* undo re-plotting

* Incorporate new ImpactFuncSet.__init__ method

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
"""
# Handle array inputs
if not np.isscalar(numerator) or not np.isscalar(denominator):
# If either input is an array and contains NaN, replace the whole result with replace_with
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems counter intuitive to me. why not safe_divide([nan, nan, 1.], 4) == [nan, nan, 0.25] ?
I don't understand why division by zero is dealt with element wise but if any of the elements is nan all should turn to nan?
Since safe_divide(4, 0) is equal to safe_divide(4, nan), i.m.o., safe_divide([4, 4], [1, 0]) should also be equal to safe_divide([4, 4], [1, nan]).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. the choice in the safe_divide function to treat NaN values in array inputs differently than division by zero is indeed not necessary. I updated the function:

` with np.errstate(divide="ignore", invalid="ignore"):
result = np.true_divide(numerator, denominator)

    # Check if the result is a scalar
    if np.isscalar(result):
        if not np.isfinite(result):
            return replace_with
    else:
        # Replace infinities, NaNs, and division by zeros in np.ndarray
        result[~np.isfinite(result)] = replace_with

return result`

Comment on lines 225 to 229
if np.any(np.isnan(numerator)) or np.any(np.isnan(denominator)):
if np.isscalar(numerator) or np.isscalar(denominator):
return replace_with
else:
return np.full(numerator.shape, replace_with)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no test for this case yet, or is there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the revised safe_divide function, the specific code section is no longer present. However, I still added tests for NaN handling.

Comment on lines 191 to 194
numerator : array-like or scalar
The numerator for division.
denominator : array-like or scalar
The denominator for division. Division by zero is handled safely.
Copy link
Collaborator

@emanuel-schmid emanuel-schmid Jan 23, 2024

Choose a reason for hiding this comment

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

there are only tests for scalar and np.ndarray, but it could be a just a list - right?

however - instead of adding tests I'd rather pin the input data to np.ndarray.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, indeed. I added a test for lists too.


Returns
-------
array-like or scalar
Copy link
Collaborator

Choose a reason for hiding this comment

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

the return type for array-like input is np.ndarray

simonameiler and others added 4 commits January 24, 2024 08:49
Co-authored-by: Emanuel Schmid <51439563+emanuel-schmid@users.noreply.github.com>
Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

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?

climada/engine/unsequa/unc_output.py Show resolved Hide resolved
climada/engine/unsequa/unc_output.py Show resolved Hide resolved
climada/engine/unsequa/calc_delta_climate.py Show resolved Hide resolved
climada/engine/unsequa/calc_delta_climate.py Show resolved Hide resolved
climada/engine/unsequa/calc_delta_climate.py Show resolved Hide resolved
climada/engine/unsequa/calc_delta_climate.py Show resolved Hide resolved
Comment on lines 559 to 566
# Check if the column data is empty or contains only NaNs
data, m_unit = u_vtm(unc_df_plt[col])
data = pd.Series(data)
if data.empty or data.isna().all() or data.dropna().shape[0] < 2:
print(f"Skipping plot for '{col}': insufficient data.")
if ax is not None:
ax.remove()
continue
Copy link
Member

Choose a reason for hiding this comment

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

Good idea, but it looks a bit weird that the subplots are missing completely. Instead of removing the axes, you could make an empty plot and write a short text like "No data to plot"?

doc/tutorial/climada_engine_unsequa.ipynb Outdated Show resolved Hide resolved
climada/engine/unsequa/calc_base.py Outdated Show resolved Hide resolved
climada/engine/unsequa/calc_base.py Outdated Show resolved Hide resolved
simonameiler and others added 3 commits January 25, 2024 09:29
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
@simonameiler
Copy link
Collaborator Author

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.

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

Thank you @simonameiler! 🙌 I fixed the remaning whitespace issues and will merge as soon as the pipeline succeeds (which it should 🤞)

@peanutfun peanutfun changed the base branch from develop to main January 31, 2024 09:17
@peanutfun peanutfun changed the base branch from main to develop January 31, 2024 09:17
@peanutfun peanutfun merged commit 7979fef into develop Jan 31, 2024
10 of 11 checks passed
@peanutfun peanutfun deleted the feature/unsequa_delta_climate branch January 31, 2024 09:40
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.

8 participants