-
Notifications
You must be signed in to change notification settings - Fork 130
Improve efficiency of local exceedance intensitry/impact and local return periods functions #1012
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
Conversation
climada/engine/impact.py
Outdated
LOGGER.info( | ||
"Some errors in the previous calculation of local exceedance impacts have been corrected," | ||
" see Impact.local_exceedance_impact. To reproduce data with the " | ||
"previous calculation, use CLIMADA v5.0.0 or less." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool idea, I am not sure though that this should be an info logging. @emanuel-schmid : what would be the most elegant way to inform the users that something changed, without having logs all the time?
climada/engine/impact.py
Outdated
impacts_stats, title, column_labels = self.local_exceedance_impact( | ||
return_periods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this calculous optional? Or what would be the way to refine the plot without having to recalculate these values (which can take quite long)? Is it using the util method instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best way is to do gdf, title, columns = hazard.local_exceedance_intensity()
and u_plot.plot_from_gdf(gdf, title, columns)
. Then you can just change the plot options by repeating the second step, without recalculating the gdf. The plot_rp_intensity
is just a wrapper around these two functions, which is why I asked if we want to keep it in the first place.
One way of not repeating the calculation here if one calls the function plot_rp_intensity
twice would be to save the local exceedance gdf as an attribute of the Hazard object, but I don't think we want to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about writing the info to better use the other two functions in the docstring, as you suggested in the issue? Is this enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be a solution I think. For small hazards I guess the method is rather fast, and for lager ones there would the solution in the docstring.
This PR is now ready for review. Note that I did not add a code example to any docstring as I think the only understandable place would be util.interpolation._group_frequency which is a private function that will not be seen by the user. |
just realized that the fix of the rounding method is much slower than the method before that had floating point problems. I will try to solve this, I'll say as soon as I'm ready |
Quick update: |
Does this mean that one cannot bin for the "interpolate" method? And does that mean that the user always has to come up with a number for the rounding? |
Yes, one cannot bin anymore in interpolation, as binning does have little effect here (it just "smoothens" the data). In extrapolation, default is not binning, and the user has to provide the decimal places to bin if they want to bin. Binning in decimals and not n_sig_sigits because it is simpler. In this way, binning has to be consciously chosen by the user, as it is also not clear what a good default value would be and if one would want to bin by default. |
Update: |
Awesome. I made some suggestions for passing the keyword arguments to the underlying method in the case of the plotting method. I hope these work as intended. Nice work! The test coverage is a bit lower now, not sure why exactly. The pre-commit checks are failing, but I do not understand why exactly. Maybe retry it. |
Co-authored-by: Chahan M. Kropf <[email protected]>
Thanks for the review! I included the suggestions and checked that they work. Also, the tutorial now uses a hazard object with the correct frequencies, without redefining frequencies (Emanuel put it on the API). Let me know if we can merge :) |
climada/hazard/plot.py
Outdated
**kwargs, | ||
): | ||
""" | ||
This function is deprecated, | ||
use Impact.local_exceedance_impact and util.plot.plot_from_gdf instead. | ||
Compute and plot hazard exceedance intensity maps for different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really only a small detail, but it seems that the entire docstring is moved one space to the right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, how did you find that? :D it doesn't show in my pylint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, it just showed weirdly on the screen in the github file-change view. Laser-eyes :D
Great, this makes the methods much clearer, faster and better tested! This is ready to merge. Before, please do not forget to update the changelog file! |
Changes proposed in this PR:
Changes address the methods
Hazard.local_exceedance_intensity
,Hazard.local_return_period
,Impact.local_exceedance_impact
, andImpact.local_return_period
.Hazard.plot_rp_intensity
andImpact.plot_rp_imp
now directly call the corresponding functions above, and a warning is added about how to recover values that are computed with CLIMADA 5.0.0 or earlier.Comments for future improvements:
calc_freq_curve
and the classImpactFreqCurve
could be adapted to also include the different methods of util.interpolation.This PR fixes #1010 and #1015
PR Author Checklist
develop
)PR Reviewer Checklist