Skip to content

Conversation

@spjuhel
Copy link
Collaborator

@spjuhel spjuhel commented Dec 17, 2025

As the Risk trajectory PR is too substantial, this is a first split.

This PR implements Snapshots as the foundational object for risk trajectories.

These objects regroup Exposures, Hazard and ImpactFuncSet instances for a specific date.

The rationale is to provide an immutable snapshot of risk. As such, Exposures, Hazard, ImpactFuncSet and date inputs are deep-copied, and "protected" behind read-only properties.

Likewise, to avoid any ambiguity regarding adaptation, i.e., as a user, if I provide an adaptation measure, should I give the base (Exposures, Hazard, ImpactFuncSet) triplet to which the measure is applied ? Or the triplet where the measure as already been applied, a measure is always None and can only be set through apply_measure() which returns a new object (with the measure set).

PR Author Checklist

PR Reviewer Checklist

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

Looks good, but I somehow still have 19 comments 🙈 The linter pipeline does not seem to work, and some comments are related to would-be linter issues.

As discussed, feel free to use pytest when writing new test modules.

We also briefly discussed writing this as a frozen dataclass, but I think the ref_only parameter would make the initialization for a dataclass too cumbersome. You can leave it as is, but there might be a way to remove the property definitions (see comment).

impfset=self.mock_impfset,
date=2023,
)
self.assertEqual(snapshot.date, datetime.date(2023, 1, 1))
Copy link
Member

Choose a reason for hiding this comment

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

Is it really useful to support ints? This result could be a bit confusing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the default thing people will use are years, so I would say yes.

Copy link
Member

Choose a reason for hiding this comment

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

But string years are compatible with pandas and numpy:

>>> pandas.Timestamp("2022")
pandas.Timestamp('2022-01-01 00:00:00')
>>> numpy.datetime64("2022")
numpy.datetime64('2022')
>>> numpy.datetime64("2022", "D")  # Force interval 'day'
numpy.datetime64('2022-01-01')

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.

2 participants