Risk Trajectory Split 1 : Snapshots #1197
Conversation
spjuhel
requested review from
chahank,
emanuel-schmid and
peanutfun
as code owners
peanutfun
left a comment
peanutfun
left a comment
There was a problem hiding this comment.
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).
| __all__ = [ | ||
| "Snapshot", | ||
| ] |
There was a problem hiding this comment.
Excellent 👏 We will need this everywhere, eventually.
climada/trajectories/snapshot.py
Outdated
| LOGGER.debug(f"Applying measure {measure.name} on snapshot {id(self)}") | ||
| exp, impfset, haz = measure.apply(self.exposure, self.impfset, self.hazard) | ||
| snap = Snapshot(exposure=exp, hazard=haz, impfset=impfset, date=self.date) | ||
| snap._measure = measure |
There was a problem hiding this comment.
This should raise a linter error? You are accessing a private attribute of a different object. Measure should be passed via the init, see comment above.
There was a problem hiding this comment.
Also: Does ref_only not apply for measure?
| def __init__( | ||
| self, | ||
| *, | ||
| exposure: Exposures, | ||
| hazard: Hazard, | ||
| impfset: ImpactFuncSet, | ||
| date: int | datetime.date | str, | ||
| ref_only: bool = False, | ||
| ) -> None: |
There was a problem hiding this comment.
You need to be able to pass measure in the init, otherwise you have to modify a private attribute when applying a measure
| @@ -0,0 +1,28 @@ | |||
| """ | |||
There was a problem hiding this comment.
You need to add this module to the docs
| self.assertEqual(new_snapshot.exposure, self.mock_modified_exposure) | ||
| self.assertEqual(new_snapshot.hazard, self.mock_modified_hazard) | ||
| self.assertEqual(new_snapshot.impfset, self.mock_modified_impfset) |
There was a problem hiding this comment.
| self.assertEqual(new_snapshot.exposure, self.mock_modified_exposure) | |
| self.assertEqual(new_snapshot.hazard, self.mock_modified_hazard) | |
| self.assertEqual(new_snapshot.impfset, self.mock_modified_impfset) | |
| self.assertIs(new_snapshot.exposure, self.mock_modified_exposure) | |
| self.assertIs(new_snapshot.hazard, self.mock_modified_hazard) | |
| self.assertIs(new_snapshot.impfset, self.mock_modified_impfset) |
| self.assertEqual(snapshot.date, date_obj) | ||
|
|
||
| def test_init_with_invalid_date(self): | ||
| with self.assertRaises(ValueError): |
There was a problem hiding this comment.
Check for exact error message
| with self.assertRaises(ValueError): | |
| with self.assertRaisesRegex(ValueError, "String must be in the format"): |
| ) | ||
|
|
||
| def test_init_with_invalid_type(self): | ||
| with self.assertRaises(TypeError): |
There was a problem hiding this comment.
Check error message (not sure my raw string is escaped correctly)
| with self.assertRaises(TypeError): | |
| with self.assertRaisesRegex(TypeError, r"date\_arg must be an int, str, or datetime\.date"): |
| impfset=self.mock_impfset, | ||
| date=2023, | ||
| ) | ||
| self.assertEqual(snapshot.date, datetime.date(2023, 1, 1)) |
There was a problem hiding this comment.
Is it really useful to support ints? This result could be a bit confusing
There was a problem hiding this comment.
Well, the default thing people will use are years, so I would say yes.
There was a problem hiding this comment.
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')
As the Risk trajectory PR is too substantial, this is a first split.
This PR implements
Snapshotsas the foundational object for risk trajectories.These objects regroup
Exposures,HazardandImpactFuncSetinstances for a specificdate.The rationale is to provide an immutable snapshot of risk. As such,
Exposures,Hazard,ImpactFuncSetanddateinputs 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 throughapply_measure()which returns a new object (with the measure set).PR Author Checklist
develop)PR Reviewer Checklist