Skip to content

Conversation

@emanuel-schmid
Copy link
Collaborator

@emanuel-schmid emanuel-schmid commented Feb 20, 2024

Changes proposed in this PR:

  • xarray 2024.2 introduced an AttributeError in year_month_day_accessor of Hazard.from_xarray_raster where: the array argument is not reallly a date-time array. This PR fixes the problem by simply adding the Exception type to the list of caught Exceptions.
  • However, the update to xarray 2024.2 also seems to slow down the procedure significantly. On a personal computer the test test_base_xarray.TestReadDefaultNetCDF.test_event_no_time took between 6 and 8 seconds where it finished in 1 to 2 seconds with version 2024.1

This PR fixes #852

PR Author Checklist

PR Reviewer Checklist


# Handle access errors
except (ValueError, TypeError) as err:
except (ValueError, TypeError, AttributeError) as err:
Copy link
Member

@peanutfun peanutfun Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct and expected. From the v2024.2 changelog:

DataArray.dt now raises an AttributeError rather than a TypeError when the data isn’t datetime-like.

Thanks for the fix!

Copy link
Collaborator

@tovogt tovogt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks fine to me, in general. However, the fact that version 2024.2 slows down the unit test by a factor 4 is quite suspicious. I would suggest to merge this, but to create a separate issue that makes note of the slowdown. I would suspect that it's possible to avoid the slowdown somehow, but I don't have time to investigate this any further.

@emanuel-schmid emanuel-schmid merged commit 27b9e80 into develop Feb 21, 2024
@emanuel-schmid emanuel-schmid deleted the feature/xarray_2024.2 branch February 21, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants