Skip to content

Conversation

spjuhel
Copy link
Collaborator

@spjuhel spjuhel commented Apr 2, 2025

Changes proposed in this PR:

Implements RiskTrajectory objects to ease the evaluation of risk / impact in between two points in time.

See the associated discussion here: #1039

A notebook showcasing this new module is available in doc/tutorial/climada_trajectory.ipynb. (It is not a real tutorial yet, but will serve as its basis).

Also note that the future option appraisal module will be based on this.

Remaining TODO s

  • Sort the list of Snapshot by their date when instantiating
  • Add a check that Exposures have the same shape within a risk trajectory.
  • Add a check that group_id are identical for Exposures or handle that case (Handled, different group_id are possible, metric is computed at each date and interpolated in between)
  • Make a decision about risk transfer (possibly after discussion with @chahank, see Integration of risk trajectories within climada #1039 ):
  • Make plots nicer, also cleanup "plot.py" file (no longer needed?)
  • Write extensive guide (see Integration of risk trajectories within climada #1039 )

PR Author Checklist

PR Reviewer Checklist

@chahank

This comment was marked as duplicate.

@chahank

This comment was marked as duplicate.

@chahank

This comment was marked as duplicate.

@chahank

This comment was marked as duplicate.

@chahank

This comment was marked as duplicate.

@spjuhel
Copy link
Collaborator Author

spjuhel commented Apr 4, 2025

I created a discussion here #1039
I think it is better than an Issue, as we will probably continue this discussion if/when we want to integrate timeseries in the future.

@chahank
Copy link
Member

chahank commented Sep 17, 2025

Small comments to the tutorial. They are really just from a user’s perspective at the moment.

  • Instead of a wall of text, maybe start with a quick example which goes within one or two cells to the results. I can help with that :)
  • Is the snapshot.measure supposed to be a MeasureSet ? Shall we then use the name measureset or similar?
  • The method snap.apply_measure() cannot apply the measure that are already in snapshot.measure ? This is a bit unintuitive. Should snapshot.measure and .apply_measure be private?
  • default_rp at [50, 100, 500] is probably a bit ambitious. Is it necessary to have a default?
  • The risk_traj.eai_exp contains the column coord_id , but risk_traj.coord_id does not exist. It is unclear what one should do with this information.
  • risk_traj.risk_periods returns a list of python pointers, so quite useless (in particular since it is a property without docstring, so I cannot make sense of that output). Should this be private?
  • risk_traj.risk_components_metrics() maybe change components to contribution ? Also, there is the index column which is unclear.
  • plot_per_date_waterfall() maybe rename plot_time_waterfall() as it is more than just per date?
  • I think the handling of the RP is a bit confusing now. There is a default set, but it is unclear how and why. Then there is a method to access them, isk_traj.return_periods_metrics which requires the RPs as input. However, the method risk_traj.per_period_risk_metrics() (and others) just compute the RP based on the default.
  • The use of the method isk_traj.npv_transform is unclear. Should this be private? Idem for risk_traj.identify_continuous_periods
  • In a future, the discounted vs not discounted risk plot might be implemented directly, looks useful.
  • Maybe we could make Enum classes to define the choices for the interpolation and impact calculations methods.

Really exciting, overall the use looks very straightforward! (edited)

Comment on lines 63 to 69
res = []
for point in range(interpolation_range):
ratio = point / (interpolation_range - 1)
mat_interpolated = mat_start * (1 - ratio) + ratio * mat_end
mat_interpolated.data = np.exp(mat_interpolated.data * np.log(rate))
res.append(mat_interpolated)
return res
Copy link
Member

Choose a reason for hiding this comment

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

Could also be a list comprehension.

Copy link
Member

Choose a reason for hiding this comment

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

And curiosity, what does res stand for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

result / return

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is a terrible name, but also not fundamental.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you handle the change of .data within a list comprehension ?

Comment on lines 40 to 47
A snapshot of exposure, hazard, and impact function at a specific date.

Attributes
----------
date : datetime
Date of the snapshot.
measure: Measure | None
The possible measure applied to the snapshot.
Copy link
Member

Choose a reason for hiding this comment

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

Probably incomplete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Properties have their own docstring if you are talking about that?

Copy link
Member

Choose a reason for hiding this comment

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

hmmmm ok, I see. This is quite confusing, since the properties are just wrapper for attributes which are not described here. Reading this, one does not know what is in the snapshot, it looks like there are only dates and measures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but this is the PEP standard.

Copy link
Member

@chahank chahank left a comment

Choose a reason for hiding this comment

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

Started with an overview of the easier parts. Will need much more time for the more complex core classes.

Comment on lines 36 to 41
from climada.trajectories.riskperiod import (
AllLinearStrategy,
CalcRiskPeriod,
ImpactCalcComputation,
ImpactComputationStrategy,
)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not import these modules directly (instead of importing them from riskperiod) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand the question?

Do you mean from riskperiod import instead of from climada.trajectories.riskperiod ?

This is virtually the same (slightly more robust). These lines are generated automatically by my LSP when a new import is required.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being unclear. I meant that for instance ImpactCalcComputation should be imported from its module, i.e. from climada.trajectories.impact_calc_strat right?

Copy link
Collaborator Author

@spjuhel spjuhel Sep 26, 2025

Choose a reason for hiding this comment

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

Oh, right! Yes indeed this is not elegant.
In any case, I think I am going to make proxies within trajectory/__init__.py to simplify imports.

LOGGER = logging.getLogger(__name__)

POSSIBLE_METRICS = ["eai", "aai", "return_periods", "risk_components", "aai_per_group"]
DEFAULT_RP = [50, 100, 500]
Copy link
Member

Choose a reason for hiding this comment

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

As raised in the discussion, I think there is a conundrum with this default. It should imho be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it a forced input? What if people are not interested in RP? Is it empty by default?

Copy link
Member

Choose a reason for hiding this comment

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

I do not undersand this response exactly.

Having arbitrary defined default RPs, which sometimes are applied, and sometimes user input RPs are applied, is confusing, as explained in the discussion to this PR. So, I would remove the default here. You can always set the default in the method that computes the values for the RPs. Then it is clear to the user. I would also choose a bit more conservative values, e.g., [20, 50, 100]. The RP500 is only rarely available in hazard datasets.

Does this clarify the response?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So how it works now is that the methods do not have RPs has argument any more (simplifies a lot) and always use the .return_periods attribute which has a default value (changed to 20, 50, 100) and can be set at init (to be coded still) or via the property afterwards (and takes care of updating the metrics).

Does that work for you?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! Important is that you can set it in the init and via properties too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated in 3a026cd

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.

4 participants