Add select method to HazardForecast #1185
Conversation
ValentinGebhart
requested review from
chahank,
emanuel-schmid and
peanutfun
as code owners
…CLIMADA-project/climada_python into add_sel_to_haz_and_imp_forecast
climada/hazard/forecast.py
Outdated
| if lead_time is not None | ||
| else np.full_like(self.lead_time, True, dtype=bool) | ||
| ) | ||
| mask_event_id = np.asarray(self.event_id)[(mask_member & mask_lead_time)] |
There was a problem hiding this comment.
Technically, this is probably not a mask anymore, but the event id you select right? Just to be sure this is the wanted behaviour (which I think it is).
There was a problem hiding this comment.
Agreed, I'll adapt the naming!
climada/hazard/test/test_forecast.py
Outdated
|
|
||
| assert haz_fc_sel.centroids == haz_fc.centroids | ||
|
|
||
| def test_derived_select(self, haz_fc, lead_time, member, haz_kwargs): |
There was a problem hiding this comment.
I would here also use the @pytest.mark.parametrize . This makes the eventual error messages much clearer.
There was a problem hiding this comment.
actually, I'm a bit confused about the parameterize. Above it seems to be for running the test multiple times with a different select variable. Here we are also testing on different combinations of select variables and their interactions, so I'm not sure how to make use of the parametrize here..
There was a problem hiding this comment.
Another way is then to make one test/function for each combination.
There was a problem hiding this comment.
One function for separate leadtime/member selection, one function for the combined selections, one function for "emtpy" selections (as discussed below)
| ) | ||
| haz_fc_select = haz_fc2.select(event_id=[1, 2, 4], member=[0]) | ||
| npt.assert_array_equal(haz_fc_select.event_id, [1, 2, 4]) | ||
| npt.assert_array_equal(haz_fc_select.member, [0, 0, 0]) |
There was a problem hiding this comment.
@peanutfun Should we add a test for "no match"?
Something like
haz_fc_empty = haz_fc.select(event_names = haz_fc.event_name[:2], member=member[-2:])
will raise
IndexError: arrays used as indices must be of integer (or boolean) type
that we could test for. Not sure if this is behaviour that we want, though, I would rather expect an empty HazardForecast. But it is the same behaviour as Hazard.select()
There was a problem hiding this comment.
You are correct, there should be a test. But we should not go further than the functionality of Hazard.select. If that's the error from the base class, I'm fine with it being raised here, too.
Changes proposed in this PR:
This PR fixes #1157
PR Author Checklist
develop)PR Reviewer Checklist