Add inter/extrapolate features to ImpactFreqCurve

[ValentinGebhart]

Changes proposed in this PR:

• add features of util interpolation module (used in local_exceedance_impact methods etc.) to the class ImpactFreqCurve, adding the interpolate method to evaluate ImpactFreqCurve at new return periods with inter- and extrapolation options
• add test for interpolate method
• renamed variable in interpolation module to avoid confusion
• add deprecation warning if user calls impact.calc_freq_curve() with given return periods as inputs

This PR fixes #

PR Author Checklist

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[chahank]

Great, small tweaks.

Changelog needs to be updated too.

[ValentinGebhart]

This is what the method plot_extrapolate would look like in the example at the bottom of the 10min intro.

imp.calc_freq_curve().plot()

imp.calc_freq_curve().plot_extrapolate(return_period_range=(1, 300), kwargs_interp={"method": "extrapolate"})

[chahank]

Nice! And how would the interpolate look like? It seems to me that for this function interpolation (with extrapolation) would be more informative to smooth out the missing values at high return periods. (Maybe this should also be the default instead of the direct extrapolation).

[ValentinGebhart]

Nice! And how would the interpolate look like?

Interpolate would look exactly the same but with no values outside the two dashed lines. I think matplotlib would the automatically zoom into the data range, so it would look very much like the original plot() method (ofc the interpolation is a bit different).

It seems to me that for this function interpolation (with extrapolation) would be more informative to smooth out the missing values at high return periods. (Maybe this should also be the default instead of the direct extrapolation).

What exactly do you mean with "smooth out"? I assume to mark the outside-of-data range, but not sure how you would like to visualize it.

[ChrisFairless]

Thanks a lot for this, it's going to be very useful for me!

Something that would be useful in some of the work I'm doing would be an ImpactFreqCurve.add_points() method, that returns a new ImpactFreqCurve with additional interpolated/extrapolated points using this new functionality.

Also: if that's not within scope of the PR and you'd just like to merge we can leave it for later!

[chahank]

Great to hear @ChrisFairless. If you need this, you could add a wrapper method around the added extroplate one. Would you like to do this before we merge? Otherwise, the new method gives back the new values which can then be used to create a new object.

[ValentinGebhart]

I am hestitating with merging the PR due to two points. What do you think @chahank?

(1) Depending on user input it can happen that the return period curve is interpolated twice, each with different methods. In this code

fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10))

fc.plot_extrapolate(return_periods=np.linspace(10,100,100))

The first line creates a frequency curve at the given return periods, interpolated with linear interpolation between (exceedence frequncy, impact) pairs.
The second line interpolates to a second series of return periods (here with higher resolution), by default with linear interpolation between (log(exceedence frequncy), log(impact)) pairs. Apart from the different default settings, I don't think the double interpolation is very elegant. If the first line is instead fc = imp.calc_freq_curve(), the above is not an issue.

(2) Is the plot style clear or do we want to show it differently? See two dashed gray lines indicating the range of data.

[chahank]

Good points!

I am hestitating with merging the PR due to two points. What do you think @chahank?

(1) Depending on user input it can happen that the return period curve is interpolated twice, each with different methods. In this code

fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10))

fc.plot_extrapolate(return_periods=np.linspace(10,100,100))

The first line creates a frequency curve at the given return periods, interpolated with linear interpolation between (exceedence frequncy, impact) pairs. The second line interpolates to a second series of return periods (here with higher resolution), by default with linear interpolation between (log(exceedence frequncy), log(impact)) pairs. Apart from the different default settings, I don't think the double interpolation is very elegant. If the first line is instead fc = imp.calc_freq_curve(), the above is not an issue.

Should we make it default that calc_freq_curve does not take specific interpolations?

(2) Is the plot style clear or do we want to show it differently? See two dashed gray lines indicating the range of data.

I think it is a bit unclear what the grey lines mean. Not sure how to make it clearer. Maybe instead make the color of the extrapolated part of the curve dashed? And no vertical dashed lines?

[ValentinGebhart]

Perfect, I like both your suggestions. I will implement them. Small question:

A line like
fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10))
would then result in an error because at this stage we just evaluate at the return periods of the impact object. The user can then inter/extrapolate later. This might break some existing user code or pipelines. Can we just change it or do we need deprecation warning etc?

[chahank]

Perfect, I like both your suggestions. I will implement them. Small question:

A line like fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10)) would then result in an error because at this stage we just evaluate at the return periods of the impact object. The user can then inter/extrapolate later. This might break some existing user code or pipelines. Can we just change it or do we need deprecation warning etc?

Good point. Then for the moment, let us keep it working, but with a deprecation warning.

[ValentinGebhart]

I added a deprecation warning if one creates the ImpactFreqCurve using Impact.calc_freq_curve() for specfic return periods, for instance like this:
fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10)).

Also, I changed the extrapolation plot style, extrapolated values are now shown as a dashed line, see plot above.

@chahank for me this would be ready to merge, let me know if we should go ahead.

[chahank]

I added a deprecation warning if one creates the ImpactFreqCurve using Impact.calc_freq_curve() for specfic return periods, for instance like this: fc = imp.calc_freq_curve(return_per=np.linspace(10, 100, 10)).

Also, I changed the extrapolation plot style, extrapolated values are now shown as a dashed line, see plot above.

@chahank for me this would be ready to merge, let me know if we should go ahead.

Excellent! We now just have one little inconsistency: the extrapolate method returns a numpy array, while the calc_freq_curve returns directly the ImpactFreqCurve. Maybe we can improve the deprecation message to make it clear how to obtain a new ImpactFreqCurve? Or should maybe the extrapolate method be able to return an ImpactFreqCurve too?

[ValentinGebhart]

I would say that there are two methods to create an ImpactFreqCurve, one is impact.calc_freq_curve(), and one is via manually initializing it.
Then, given an ImpactFreqCurve, one can inter- and extrapolate to any test return period using extrapolate(), returning an array of corresponding impact values.
For me, this behaviour would be okay, as it treats the actual "data-based" frequency curve as the central object, and does not equate it with some extrapolated values.

If you think it would be better to have the extrapolate method take an ImpactFreqCurve and return an ImpactFreqCurve, would could also do that.

[chahank]

I would say that there are two methods to create an ImpactFreqCurve, one is impact.calc_freq_curve(), and one is via manually initializing it. Then, given an ImpactFreqCurve, one can inter- and extrapolate to any test return period using extrapolate(), returning an array of corresponding impact values. For me, this behaviour would be okay, as it treats the actual "data-based" frequency curve as the central object, and does not equate it with some extrapolated values.

If you think it would be better to have the extrapolate method take an ImpactFreqCurve and return an ImpactFreqCurve, would could also do that.

Yes, I am unsure what is best. Maybe we can ask in the developer's call for an opinion?

[ValentinGebhart]

After advice from @peanutfun and @emanuel-schmid (thanks!), I changed the implementation to:

• freq_curve = impact.calc_freq_curve() returns an ImpactFreqCurve. If return periods are given as an argument, a deprecation warning is raised.
• freq_curve_interp = freq_curve.interpolate(return_periods=np.linspace(10, 300, 100), method="extrapolate") returns a new ImpactFreqCurve object with the inter- and extrapolated impacts at the given return periods. Interpolation is default.
• freq_curve.plot() is the original plot function and can be used by the user to combine curves, e.g.

freq_curve_extra = freq_curve.interpolate(return_periods=np.linspace(10,300,100), method="extrapolate")
freq_curve_inter = freq_curve.interpolate(return_periods=np.linspace(10,300,100))
ax = freq_curve_inter.plot(color="blue")
freq_curve_extra.plot(axis=ax, linestyle="--", color="blue")

@chahank Let me know if this looks good, then we can merge.