{{ message }}
Add local_return_period method to Impact objects and new tutorial#971
Merged
Conversation
spjuhel
reviewed
Nov 6, 2024
spjuhel
left a comment
Collaborator
There was a problem hiding this comment.
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
HazardandImpactobjects. 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)"
Collaborator
Author
spjuhel
approved these changes
Nov 7, 2024
spjuhel
left a comment
Collaborator
There was a problem hiding this comment.
Nice! Happy to approve this PR :)
sarah-hlsn
approved these changes
Nov 19, 2024
sarah-hlsn
left a comment
Collaborator
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Changes proposed in this PR:
climada.engine.impact.Impact.local_return_periodthat computes locally (i.e. per centroid) return periods for given threshold impact values (inputs to the method). Functionality analogous toclimada.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.doc.tutorial.climada_util_local_exceedance_values.ipynbthat explains what is done in the methodsHazard.local_exceedance_intensity,Hazard.local_return_period,Impact.local_exceedance_impact, andImpact.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 exemplaryHazardand an exemplaryImpactobject.PR Author Checklist
develop)PR Reviewer Checklist