-
Notifications
You must be signed in to change notification settings - Fork 150
Risk Trajectory Split 1 : Snapshots #1197
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
base: develop
Are you sure you want to change the base?
Conversation
peanutfun
left a comment
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')
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