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

Add local_return_period method to Impact objects and new tutorial #971

Merged
merged 9 commits into from
Nov 20, 2024

Conversation

ValentinGebhart
Copy link
Collaborator

@ValentinGebhart ValentinGebhart commented Nov 5, 2024

Changes proposed in this PR:

  • New method climada.engine.impact.Impact.local_return_period that computes locally (i.e. per centroid) return periods for given threshold impact values (inputs to the method). Functionality analogous to climada.hazard.base.Hazard.local_return_period (underlying functions already exist, see PR Combining several interpolation functions using a new util function #918), but with impact values instead of intensity values.
  • New tutorial doc.tutorial.climada_util_local_exceedance_values.ipynb that explains what is done in the methods Hazard.local_exceedance_intensity, Hazard.local_return_period, Impact.local_exceedance_impact, and Impact.local_return_period. First, a dummy hazard object is used to showcase the different settings of the methods. Then, the methods are applied to an exemplary Hazard and an exemplary Impact object.

PR Author Checklist

PR Reviewer Checklist

Copy link
Collaborator

@spjuhel spjuhel left a comment

Choose a reason for hiding this comment

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

Very nice work! 👍

I have some minor comments on the tutorial, mostly typos/rephrasing, and maybe some clarification in the first part with the mock example hazard.

  • "This is a tutorial to present and explain certain methods avaibale in Hazard and Impact objects. These methods" -> "This tutorial presents methods available in ... to compute ..."
  • Cell "Compute local exceedance intensities": "We first sort the centroid's intensities in descending order"
  • -- : "100****years"
  • -- : First centroid D then centroid C? Typo?
  • -- : It took some time for me to understand. Maybe just briefly stating why we cumulate the frequencies, linking to exceedence.
  • -- : last line "how do we want to estimate" (?)
  • Cell "Option 1 (default setting)": "(here, 30 years)", same for "5 and 150" further in the sentence
  • -- : I would swap the last two sentences on logarithmic scaling, to get the explanation right next to it (and remove the "however").
  • Cell "option 2": the user wants to guess -> estimate
  • -- : "Also centroid with events intensity will be assigned zero exceedance intensity." Not sure what you mean here.
  • Cell "option 4": "Here, return periods will be assigned the exceedance intensity of the next smaller of the observed return periods." -> I found this ambiguous. Is it the closest return period that is smaller or the smaller return period that is above? Maybe rephrase?
  • Cell "Compute local return periods" : Rephrase. Maybe in the form "Using you can find/plot the return period of events exceeding different intensities"
  • Comparison of the described methods on a realistic ... : real example rather than realistic (being extra picky here ahah)
  • "for 'test' return periods of" -> why 'test' ?
  • "for a return period range from 5 to 250 years" -> either "for return periods ranging from" or "for a return period range of"
  • "lie outside the range of observed values for this centroid (blue scatter points)"

@ValentinGebhart
Copy link
Collaborator Author

ValentinGebhart commented Nov 6, 2024

@spjuhel Thanks a lot for the comments, I have implemented them all. If you want, you can check what I added as a brief explanation about why we sort the intensities und add up their frequencies in this specific way (second paragraph of Section Compute local exceedance intensities). I hope this makes things clearer :)

Copy link
Collaborator

@spjuhel spjuhel left a comment

Choose a reason for hiding this comment

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

Nice! Happy to approve this PR :)

Copy link
Collaborator

@sarah-hlsn sarah-hlsn left a comment

Choose a reason for hiding this comment

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

This is a great tutorial! It really takes the user by the hand and goes through the different methods step-by-step. I particularly like the inclusion of a very simple mock hazard alongside real hazard objects. I have spotted some minor typo/grammar issues. Otherwise happy to approve if all checks are working. Here a couple of propsed changes:

  • "Further below, we apply the methods to more real Hazard and Impact objects." either real --> realistic or more real --> real
  • "The Hazard object consists of two events and has a spatial extend" extend --> extent
  • "This resulting cumulative frequency for each intensity then yields the intensitiy's " intensitiy's --> intensity's

@ValentinGebhart ValentinGebhart merged commit f756f47 into develop Nov 20, 2024
19 checks passed
@ValentinGebhart ValentinGebhart deleted the feature/rp_impact branch November 21, 2024 06:49
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.

3 participants