Add better support for using custom time coordinates in temporal APIs #415
Conversation
- Update tests - Update docstring for dropping leap days with high freq - Update `KeyError` when bounds are not found for an axis
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 1260 1266 +6
=========================================
+ Hits 1260 1266 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Hey @pochedls, this PR is review. It is a pretty small change related to using custom time coordinates in temporal APIs (#391 (comment)).
| first_time_coord = dv[self.dim].values[0] | ||
| is_decoded = isinstance(first_time_coord, cftime.datetime) or isinstance( | ||
| first_time_coord, np.datetime64 | ||
| ) | ||
| if not is_decoded: | ||
| raise ValueError( | ||
| f"The time coordinates type is {type(first_time_coord)}, not " | ||
| "'cftime.datetime' or 'np.datetime64'. Time coordinates must be " | ||
| "decoded as datetime before using TemporalAccessor methods. " | ||
| ) |
There was a problem hiding this comment.
We determine if time is decoded by checking the type of the first time coordinate.
| # Get the `cftime` date type based on the CF calendar attribute. | ||
| # The date type is used to get the correct cftime.datetime sub-class | ||
| # type for creating new grouped time coordinates for averaging. | ||
| try: | ||
| self.calendar = dv[self.dim].encoding["calendar"] | ||
| self.date_type = get_date_type(self.calendar) | ||
| except KeyError: | ||
| raise KeyError( | ||
| f"The 'calendar' encoding attribute is not set on the '{data_var}' " | ||
| f"time coordinate variable ({self.dim}). This might indicate that the " | ||
| "time coordinates were not decoded, which is required for temporal " | ||
| "averaging operations. " | ||
| self.calendar = "standard" | ||
| logger.warning( | ||
| f"'{self.dim}' does not have a calendar encoding attribute set, " | ||
| "which is used to determine the `cftime.datetime` object type for the " | ||
| "output time coordinates. Defaulting to CF 'standard' calendar. " | ||
| "Otherwise, set the calendar type (e.g., " | ||
| "ds['time'].encoding['calendar'] = 'noleap') and try again." | ||
| ) | ||
|
|
||
| self.date_type = get_date_type(self.calendar) |
There was a problem hiding this comment.
If no calendar attr is set, calendar is defaulted to "standard" and a logger warning is raised (instead of a KeyError). Calendar is required for determining the cftime.datetime object type and checking if leap days should be dropped for high freq climatology/departures calculations.
| * "day": groups by (month, day) for the daily cycle departures. | ||
| If the CF calendar type is ``"gregorian"``, | ||
| ``"proleptic_gregorian"``, or ``"standard"``, leap days (if | ||
| present) are dropped to avoid inconsistencies when calculating | ||
| climatologies. Refer to [2]_ for more details on this | ||
| implementation decision. |
There was a problem hiding this comment.
Add more information about dropping leap days with high freq climos/departs.
| f"No bounds data variables were found for the '{axis}' axis. Make sure " | ||
| "the dataset has bound data vars and their names match the 'bounds' " | ||
| "attributes found on their related time coordinate variables. " | ||
| "Alternatively, you can add bounds with `xcdat.add_missing_bounds` " | ||
| "or `xcdat.add_bounds`." | ||
| ) |
There was a problem hiding this comment.
Updated KeyError message when bounds are not found for an axis.
There was a problem hiding this comment.
This looks good to me (and solves the initial issue / discussion item).
The only thing I got caught up on was the changes to _set_data_var_attrs(). Do you think the top line documentation for this function should be augmented, e.g.,
Set data variable metadata as object attributes and checks whether the time axis is decoded.
It seems like this function is partially used to enforce this (prior to calling key temporal methods).
Thanks for that catch. I just updated the docstring based on your feedback. I was considering splitting out the check for decoded time as a separate method to keep the implementation cleaner. However, it requires looking at the data variable values which means it's not a big deal to keep it inside |
Description
Summary of Changes
TemporalAccessorcalendarattr is set, calendar is defaulted to"standard"and a logger warning is raised (instead of a KeyError). Calendar is required for determining thecftime.datetimeobject type and checking if leap days should be dropped for high freq climatology/departures calculations.KeyErrorwhen bounds are not found for an axisChecklist
If applicable: