Change default Hazard.event_name to string of event_id #795

sarah-hlsn · 2023-10-23T11:26:37Z

Changes proposed in this PR:

changed default Hazard.event_name to string representation of event_id
changed Hazard test_base_xarray.py accordingly

This PR fixes #789

PR Author Checklist

PR Reviewer Checklist

Change event_name default for Hazard.from_xarray_raster to string of event id. Change variable description in class method.

adapted event_name in test_assert_default_values and deleted event_name tests from test_missing_dims since the time dimension does not affect event_name anymore

peanutfun · 2023-10-24T08:50:44Z

climada/hazard/base.py

@@ -840,7 +840,7 @@ def maybe_repeat(values: np.ndarray, times: int) -> np.ndarray:
                    None,
                    np.ones(num_events),
                    np.array(range(num_events), dtype=int) + 1,
-                    list(data[coords["event"]].values),
+                    [str(x) for x in np.array(range(num_events), dtype=int) + 1],


This does actually not behave as expected: If the dataset supplies custom event IDs (like 100...100+X), this will still set the event name to values 1...X. However, I don't see a way of inserting the event ID data as default value here. @chahank, what do you think?

Good question, but I think it does not matter that much. I was actually expecting that this PR would change the Hazard.init to make a good default instead of just the read xarray datasets method. But, this might be a larger endeavor.

Also, you do not need to cast the generator into an array (this is actually very sub-optimal, both memory-wise, speed-wise and style-wise). Also, range already provides int type. Keeping the original values, this would be it:

Suggested change

[str(x) for x in np.array(range(num_events), dtype=int) + 1],

[str(x) for x in range(1, num_events+ 1)],

Another side-note to @peanutfun : why do you use a list here instead of a dictionary? How do you keep track of the correct ordering? Is there a particular reason to use a list?

@chahank We could indeed adjust the Hazard.__init__ to handle that. I think this would be the cleanest solution. However, changing the default initialization of this class might have some repercussions elsewhere.

why do you use a list here instead of a dictionary?

Not sure I understand. The whole thing is a dictionary which is inserted into a dataframe. This transforms the dictionary keys into column names and dictionary values into the column series. Order is preserved. I agree that it might be a bit less confusing if we used DataFrame.from_records, where the dataframe can be initialized row-wise. But I recall you insisted that I create a separate variable DEF_DATA_VARS instead of hardcoding the data variables here, so I thought this was the cleanest approach.

@chahank We could indeed adjust the Hazard.init to handle that. I think this would be the cleanest solution. However, changing the default initialization of this class might have some repercussions elsewhere.

Agreed. This will come later when we change the hazard attributes to a better data format I suppose.

Not sure I understand.

Sorry that I was unclear! What I meant is why is the default_var variable a list (because then the ordering matters in that list, which is not directly obvious)?

Agreed. This will come later when we change the hazard attributes to a better data format I suppose.

Then we need an alternative. I would not like to have event_id set to 100..100+X and event_name to 1..1+X. My proposal is then to simply leave the event name empty, and let the Hazard initializer handle these cases in the future.

What I meant is why is the default_var variable a list (because then the ordering matters in that list, which is not directly obvious)?

Still unclear. Do you mean default_value? Yes, of course ordering matters, because the list relates to the DEF_DATA_VARS. Again, I can change initialization to row-wise, that would probably make it less confusing.

Then we need an alternative. I would not like to have event_id set to 100..100+X and event_name to 1..1+X. My proposal is then to simply leave the event name empty, and let the Hazard initializer handle these cases in the future.

I think having an empty hazard event is not an option, this would break some functionalities. At the moment, the initialization does NOT handle empty event_names properly, so we would have to wait until we fix that (or do it now).

Still unclear. Do you mean default_value? Yes, of course ordering matters, because the list relates to the DEF_DATA_VARS. Again, I can change initialization to row-wise, that would probably make it less confusing.

Exactly, the ordering of the values for default_value at that point in the code is not obvious. But it is also not a big deal. Maybe just a comment referring to DEF_DATA_VARS for the ordering/content?

I think having an empty hazard event is not an option, this would break some functionalities. At the moment, the initialization does NOT handle empty event_names properly, so we would have to wait until we fix that (or do it now).

Okay, then I suggest the following: We create a follow-up issue to update the default initialization of Hazard. For now, we keep the functionality of from_xarray_raster as is, i.e. event_name stores the date. We just make sure that the event_name is indeed a string (because we should always be able to write those). We can do that by calling dt.strftime on the array and storing the result.

Maybe just a comment referring to DEF_DATA_VARS for the ordering/content?

Will do.

Okay, then I suggest the following: We create a follow-up issue to update the default initialization of Hazard. For now, we keep the functionality of from_xarray_raster as is, i.e. event_name stores the date. We just make sure that the event_name is indeed a string (because we should always be able to write those). We can do that by calling dt.strftime on the array and storing the result.

Good idea! Let us do this then. @sarah-hlsn can you make the event_name to be the string version of the date as suggested by @peanutfun ?

Yes, will do!

climada/hazard/test/test_base_xarray.py

peanutfun · 2023-11-10T16:09:59Z

@sarah-hlsn This is coming along very nicely! Could you add an explicit check for event_name in test_2D_time? I am not sure if calling list on the DataArray will always result in a "flat" list. Maybe we have to create the list from <arr>.values.flat.

chahank · 2023-11-17T09:11:16Z

@sarah-hlsn : could you then please update the changelog too? Then we can soon merge this bugfix.

sarah-hlsn · 2023-11-17T09:15:06Z

@chahank Yes will do, I am currently working on the additional test suggested by @peanutfun but I may need some more time because it is not working as I expected

peanutfun · 2023-11-17T09:47:20Z

@sarah-hlsn Let me know if you need any help

chahank · 2023-11-17T12:42:27Z

CHANGELOG.md

@@ -14,6 +14,7 @@ Code freeze date: YYYY-MM-DD

 ### Changed

+- Change default event name to string of event date when importing Hazard data from xarray [#795](https://github.com/CLIMADA-project/climada_python/pull/795)


Could you please also add under fixed the issue that was fixed with this pull request please?

I think just mentioning this under Fixed is enough

chahank · 2023-11-17T13:49:42Z

Looks good to me. @peanutfun merge?

peanutfun

Very good @sarah-hlsn! 🙌 I only have a few stylistic suggestions.

CHANGELOG.md

peanutfun · 2023-11-17T15:42:22Z

CHANGELOG.md

@@ -14,6 +14,7 @@ Code freeze date: YYYY-MM-DD

 ### Changed

+- Change default event name to string of event date when importing Hazard data from xarray [#795](https://github.com/CLIMADA-project/climada_python/pull/795)


I think just mentioning this under Fixed is enough

CHANGELOG.md

climada/hazard/test/test_base_xarray.py

peanutfun · 2023-11-17T15:47:23Z

@chahank, would you go trough my suggestions and merge afterwards?

Co-authored-by: Lukas Riedel <[email protected]>

chahank · 2023-11-20T09:09:47Z

Thanks @sarah-hlsn and @peanutfun !

sarah-hlsn added 2 commits October 20, 2023 15:53

Update Hazard base.py

b54a62f

Change event_name default for Hazard.from_xarray_raster to string of event id. Change variable description in class method.

Update test_base_xarray.py

661b2c0

adapted event_name in test_assert_default_values and deleted event_name tests from test_missing_dims since the time dimension does not affect event_name anymore

peanutfun reviewed Oct 24, 2023

View reviewed changes

sarah-hlsn added 3 commits November 8, 2023 11:46

Merge branch 'develop' into fix_hazard_event_name_2.0

1cc4502

change default name to string of date

77d2727

save event_name as string of date and time

30464ee

sarah-hlsn added 3 commits November 17, 2023 11:03

ensure list of event names is flat

c6d83bc

Update CHANGELOG.md

e3c449e

Update CHANGELOG.md

c68018a

chahank reviewed Nov 17, 2023

View reviewed changes

sarah-hlsn added 2 commits November 17, 2023 14:33

Merge branch 'develop' into fix_hazard_event_name_2.0

6d047a6

update changelog

9affce6

peanutfun requested changes Nov 17, 2023

View reviewed changes

chahank and others added 2 commits November 20, 2023 10:01

Apply suggestions from code review

465ce37

Co-authored-by: Lukas Riedel <[email protected]>

Update AUTHORS.md

678a173

chahank merged commit 05ee576 into develop Nov 20, 2023
3 of 5 checks passed

emanuel-schmid deleted the fix_hazard_event_name_2.0 branch November 20, 2023 15:17

peanutfun mentioned this pull request Dec 4, 2023

Hazard.from_raster_xarray stores native values of dates in Hazard.event_name #789

Closed

This was referenced Dec 13, 2023

Add module for computing river flood footprints from GloFAS river discharge data CLIMADA-project/climada_petals#64

Merged

Hazard.from_xarray_raster requires that "event" dimension can be interpreted as datetime #829

Closed

peanutfun mentioned this pull request Jan 15, 2024

Allow reading Hazard events that are not dates from xarray #837

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default Hazard.event_name to string of event_id #795

Change default Hazard.event_name to string of event_id #795

sarah-hlsn commented Oct 23, 2023 •

edited

Loading

peanutfun Oct 24, 2023

chahank Oct 31, 2023 •

edited

Loading

peanutfun Oct 31, 2023

chahank Oct 31, 2023 •

edited

Loading

peanutfun Nov 3, 2023

chahank Nov 3, 2023

chahank Nov 3, 2023

peanutfun Nov 7, 2023

chahank Nov 7, 2023

sarah-hlsn Nov 7, 2023

peanutfun commented Nov 10, 2023

chahank commented Nov 17, 2023

sarah-hlsn commented Nov 17, 2023

peanutfun commented Nov 17, 2023

chahank Nov 17, 2023

peanutfun Nov 17, 2023

chahank commented Nov 17, 2023

peanutfun left a comment

peanutfun Nov 17, 2023

peanutfun commented Nov 17, 2023

chahank commented Nov 20, 2023

	[str(x) for x in np.array(range(num_events), dtype=int) + 1],
	[str(x) for x in range(1, num_events+ 1)],

		@@ -14,6 +14,7 @@ Code freeze date: YYYY-MM-DD

		### Changed

		- Change default event name to string of event date when importing Hazard data from xarray [#795](https://github.com/CLIMADA-project/climada_python/pull/795)

Change default Hazard.event_name to string of event_id #795

Change default Hazard.event_name to string of event_id #795

Conversation

sarah-hlsn commented Oct 23, 2023 • edited Loading

PR Author Checklist

PR Reviewer Checklist

Choose a reason for hiding this comment

chahank Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahank Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peanutfun commented Nov 10, 2023

chahank commented Nov 17, 2023

sarah-hlsn commented Nov 17, 2023

peanutfun commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahank commented Nov 17, 2023

peanutfun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peanutfun commented Nov 17, 2023

chahank commented Nov 20, 2023

sarah-hlsn commented Oct 23, 2023 •

edited

Loading

chahank Oct 31, 2023 •

edited

Loading

chahank Oct 31, 2023 •

edited

Loading