Allow reading Hazard events that are not dates from xarray by peanutfun · Pull Request #837 · CLIMADA-project/climada_python

peanutfun · 2024-01-15T10:30:23Z

Changes proposed in this PR:

Try interpreting values of the event coordinate as dates or ordinals for default values of Hazard.date. If that fails, issue a warning and set default values to zeros.
Try interpreting values of the event coordinate as dates for default values of Hazard.event_name. If that fails, issue a warning and set default values to empty strings.
Update tests.

The method still tries to read the values as dates but issues a warning if that does not work. The fallback is zeros in case of Hazard.date and empty strings for Hazard.event_name.

This extends #795, which actually introduced tighter restrictions for the 'event' coordinate. This causes several issues in my new flood module, see CLIMADA-project/climada_petals#64.

This fixes #829.

PR Author Checklist

PR Reviewer Checklist

* Set default value of `Hazard.event_name` to empty string. * Try interpreting values of the event coordinate as dates or ordinals for default values of `Hazard.date`. If that fails, issue a warning and set default values to zeros. * Update tests.

climada/hazard/base.py

…thub.com/CLIMADA-project/climada_python into feature/hazard-xarray-read-no-time-event

peanutfun · 2024-01-15T10:53:26Z

Failing tests have nothing to do with this PR

chahank · 2024-01-15T12:10:50Z

climada/hazard/base.py

+            * ``date``: The ``event`` coordinate interpreted as date or ordinal, or
+              zeros if that fails (which will issue a warning).


I am not sure about the value 0 since this is then not a valid ordinal (must be larger equal to 1). Thus, one would get a non-valid event_date which then creates easily problems down the line when a method expects an ordinal. This then might lead to hard-to-debug/understand error messages for users. Any other idea?

You are correct. I actually messed up the test. It is fixed now. I also did not realize that 0 does not work as ordinal. I now chose 1 as default value, the ordinal of date 01.01.0001.

chahank · 2024-01-15T12:11:14Z

climada/hazard/base.py

          and Examples) before loading the Dataset as Hazard.
        * Single-valued data for variables ``frequency``. ``event_name``, and
          ``event_date`` will be broadcast to every event.
+        * The ``event`` dimension need not be a time or date.


I do not understand what this is meaning here.

Yes, could be clearer. This way, maybe?

Suggested change

* The ``event`` dimension need not be a time or date.

* The values of the ``event`` coordinate need not be times or dates.

chahank · 2024-01-15T12:11:43Z

climada/hazard/base.py

          read from the Dataset. Use the method parameters to set these attributes.
        * This method does not read coordinate system metadata. Use the ``crs`` parameter
          to set a custom coordinate system identifier.
-        * This method **does not** read lazily. Single data arrays must fit into memory.


Does it load lazily now? That would be great.

It has been doing so for quite a while, I just missed deleting this line 🙃

chahank · 2024-01-15T12:14:37Z

climada/hazard/base.py

+            * ``event_name``: String representation of the event date or empty strings
+              if that fails (which will issue a warning).


Just to be sure I understand: if the data contains a field 'name', it will be ignored by default?

This list only specifies the default values. These are chosen if the dataset does not contain a field event_name OR the user chose to ignore it via read_xarray_raster(..., data_vars=dict(event_name=""))

chahank · 2024-01-15T12:16:10Z

climada/hazard/test/test_base_xarray.py

+            # Integers
+            time = np.arange(size)
+            dataset["time"] = time
+            self.time = ["1970-01-01", "1970-01-02"]  # These will be 0, 1 as ordinals


I think ordinals must be larger equal to 1. Is the comment thus incorrect?

Sorry, the test was wrong. You are correct.

chahank · 2024-01-15T12:17:34Z

Great, good improved fix. A few comments mostly for clarity and one main question: is it smart to use a non-valid ordinal as the default date?

chahank · 2024-01-16T09:54:21Z

climada/hazard/base.py

          and Examples) before loading the Dataset as Hazard.
        * Single-valued data for variables ``frequency``. ``event_name``, and
          ``event_date`` will be broadcast to every event.
+        * The ``event`` dimension need not be a time or date.


Suggested change

* The ``event`` dimension need not be a time or date.

* The values of the ``event`` coordinates can be of any type (times, dates, strings, ...)

chahank · 2024-01-16T09:56:41Z

Suggestion for the intro docstring wording:

This method reads data that can be interpreted using three coordinates: event,
        latitude, and longitude. The three coordinates to be read can be
        specified via the ``coordinate_vars`` parameter. The data and the coordinates themselves may be organized
        in arbitrary dimensions in the Dataset (e.g. four dimensions 'year', 'month',
        'day' and 'altitude' for the coordinate 'event').  See Notes and Examples if you
        want to load single-event data that does not contain an event dimension.

chahank · 2024-01-16T09:57:10Z

Excellent work! Ready to merge upon consideration of the suggested docstring changes.

peanutfun · 2024-01-16T10:33:48Z

@chahank Please have another look at eeda6b3 and merge if you concur.

climada/hazard/base.py

peanutfun added 2 commits January 15, 2024 11:12

Try to read event coordinate as date

c8d5b7f

peanutfun requested review from chahank and sarah-hlsn January 15, 2024 10:30

peanutfun commented Jan 15, 2024

View reviewed changes

climada/hazard/base.py Outdated Show resolved Hide resolved

peanutfun added 3 commits January 15, 2024 11:32

Update climada/hazard/base.py

5566141

Apply formatter to test_base_xarray.py

3cbab6d

Merge branch 'feature/hazard-xarray-read-no-time-event' of https://gi…

5892d11

…thub.com/CLIMADA-project/climada_python into feature/hazard-xarray-read-no-time-event

chahank reviewed Jan 15, 2024

View reviewed changes

peanutfun added 3 commits January 15, 2024 16:58

Set default ordinal to 1, fix tests

61282f0

Fix linter warnings

f3704f1

Switch back to class setup for xarray tests

3ef93a5

chahank reviewed Jan 16, 2024

View reviewed changes

peanutfun added 2 commits January 16, 2024 11:30

Clarify docstring of Hazard.from_xarray_raster

eeda6b3

Update CHANGELOG.md

2d663dc

chahank reviewed Jan 16, 2024

View reviewed changes

climada/hazard/base.py Show resolved Hide resolved

Update climada/hazard/base.py

0e4f275

chahank merged commit 7e9b4f8 into develop Jan 16, 2024

chahank deleted the feature/hazard-xarray-read-no-time-event branch January 16, 2024 11:03

peanutfun mentioned this pull request Jan 30, 2024

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

Closed

peanutfun mentioned this pull request Feb 23, 2024

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

Merged

4 tasks

		* ``date``: The ``event`` coordinate interpreted as date or ordinal, or
		zeros if that fails (which will issue a warning).

	* The ``event`` dimension need not be a time or date.
	* The values of the ``event`` coordinate need not be times or dates.

		* ``event_name``: String representation of the event date or empty strings
		if that fails (which will issue a warning).

Conversation

peanutfun commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Author Checklist

PR Reviewer Checklist

Uh oh!

Uh oh!

peanutfun commented Jan 15, 2024

Uh oh!

chahank Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peanutfun Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peanutfun Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chahank commented Jan 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chahank commented Jan 16, 2024

Uh oh!

chahank commented Jan 16, 2024

Uh oh!

peanutfun commented Jan 16, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

peanutfun commented Jan 15, 2024 •

edited

Loading

chahank Jan 15, 2024 •

edited

Loading

peanutfun Jan 15, 2024 •

edited

Loading

peanutfun Jan 15, 2024 •

edited

Loading