Drop leap days based on CF calendar type to calculate daily climatologies and departures #350

tomvothecoder · 2022-09-21T18:44:26Z

Description

Summary of Changes

Closes [Bug]: Drop leap years for high frequency input data when calculating climatology/departures #337
Closes Climatology calculation for daily or higher temporal frequency input #332
Closes [Doc]: Calculating Daily Climatology and Departures from Monthly Time Series needs to use daily input #303
Includes additional refactoring to keep time bounds variable deterministic in the temporal averaging accessor class.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

If applicable:

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass with my changes (locally and CI/CD build)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

tomvothecoder

Hi @lee1043, @msahn, and @pochedls (optional if you want to review), this PR is ready for review. I commented areas of interest, and you can ignore other changes because they are mostly refactoring.

The climatology and departures notebook has been fixed to use hourly data for hourly calculations. The changes can be previewed here:
https://xcdat.readthedocs.io/en/feature-337-climo-highfreq/examples/climatology-and-departures.html#

Thanks!

tomvothecoder · 2022-09-21T22:34:23Z

xcdat/temporal.py

+    def _drop_leap_days(self, ds: xr.Dataset):
+        """Drop leap days from time coordinates.

-        This method concatenates the strings in each sublist to form a
-        a flat list of custom season strings
+        This method is used to drop 2/29 from leap years (if present) before
+        calculating climatology/departures for high frequency time series data
+        to avoid `cftime` breaking (`ValueError: invalid day number provided
+        in cftime.DatetimeProlepticGregorian(1, 2, 29, 0, 0, 0, 0,
+        has_year_zero=True`).

        Parameters
        ----------
-        custom_seasons : List[List[str]]
-            List of sublists containing month strings, with each sublist
-            representing a custom season.
+        ds : xr.Dataset
+            The dataset.

        Returns
        -------
-        Dict[str, List[str]]
-           A dictionary with the keys being the custom season and the
-           values being the corresponding list of months.
-
-        Raises
-        ------
-        ValueError
-            If exactly 12 months are not passed in the list of custom seasons.
-        ValueError
-            If a duplicate month(s) were found in the list of custom seasons.
-        ValueError
-            If a month string(s) is not supported.
+        xr.Dataset
        """
-        predefined_months = list(MONTH_INT_TO_STR.values())
-        input_months = list(chain.from_iterable(custom_seasons))
-
-        if len(input_months) != len(predefined_months):
-            raise ValueError(
-                "Exactly 12 months were not passed in the list of custom seasons."
-            )
-        if len(input_months) != len(set(input_months)):
-            raise ValueError(
-                "Duplicate month(s) were found in the list of custom seasons."
-            )
-
-        for month in input_months:
-            if month not in predefined_months:
-                raise ValueError(
-                    f"The following month is not supported: '{month}'. "
-                    f"Supported months include: {predefined_months}."
-                )
-
-        c_seasons = {"".join(months): months for months in custom_seasons}
-
-        return c_seasons
+        ds = ds.sel(  # type: ignore
+            **{self._dim: ~((ds.time.dt.month == 2) & (ds.time.dt.day == 29))}
+        )
+        return ds


This is the new method to handle dropping leap years when calculating daily climatology/departures.

tomvothecoder · 2022-09-21T22:36:37Z

xcdat/temporal.py

+        if (
+            self._freq == "day"
+            and self._mode in ["climatology", "departures"]
+            and self.calendar in ["gregorian", "proleptic_gregorian", "standard"]
+        ):
+            ds = self._drop_leap_days(ds)


The logic to check whether or not to drop leap years.

tomvothecoder · 2022-09-21T22:37:18Z

xcdat/temporal.py

+            * "day": groups by (month, day) for the daily cycle departures. Leap
+              days (if present) are dropped if the CF calendar type is
+              ``"gregorian"``, ``"proleptic_gregorian"``, or ``"standard"``


Docstring update to inform users when leap days are dropped.

tomvothecoder · 2022-09-21T22:37:43Z

xcdat/temporal.py

@@ -784,6 +790,88 @@ def _set_obj_attrs(
        else:
            self._season_config["custom_seasons"] = self._form_seasons(custom_seasons)

+    def _form_seasons(self, custom_seasons: List[List[str]]) -> Dict[str, List[str]]:


_form_seasons was simply reordered, no changes were made.

tomvothecoder · 2022-09-21T22:38:10Z

tests/test_temporal.py

@@ -1382,6 +1383,97 @@ def test_weighted_daily_climatology(self):

        assert result.identical(expected)

+    def test_weighted_daily_climatology_drops_leap_days_with_matching_calendar(self):


New test for daily climatology.

tomvothecoder · 2022-09-21T22:38:17Z

tests/test_temporal.py

@@ -1606,6 +1698,126 @@ def test_unweighted_seasonal_departures_with_JFD(self):

        assert result.identical(expected)

+    def test_weighted_daily_departures_drops_leap_days_with_matching_calendar(self):


New test for daily departures.

tomvothecoder · 2022-09-21T22:38:49Z

tests/test_temporal.py

@@ -1654,7 +1866,7 @@ def test_weights_for_yearly_averages(self):
            )

            # Compare result of the method against the expected.
-            result = ds.temporal._get_weights()
+            result = ds.temporal._get_weights(ds.time_bnds)


Ignore these changes to _get_weights() (result of refactoring).

- Organize methods based on stack call - Move `self._labeled_time` instantiation to `departures()` and `_group_average()` to avoid regenerating when calculating weights and fix issue where it was not being regenerated with seasonal climo and custom seasonal climo calls in succession

codecov-commenter · 2022-09-21T22:56:45Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (298c7bc) compared to base (d37e052).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #350   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1202      1210    +8     
=========================================
+ Hits          1202      1210    +8

Impacted Files	Coverage Δ
xcdat/temporal.py	`100.00% <100.00%> (ø)`

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

…sion with diurnal cycle

lee1043

I think this PR can be merged. @tomvothecoder thank you for the update!

tomvothecoder added type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. type: enhancement New enhancement request type: docs Updates to documentation Priority: High labels Sep 21, 2022

tomvothecoder self-assigned this Sep 21, 2022

tomvothecoder changed the title ~~Drop leap days to calculate daily climos and departs~~ Drop leap days to calculate daily climatologies and departures (based on calendar type) Sep 21, 2022

tomvothecoder changed the title ~~Drop leap days to calculate daily climatologies and departures (based on calendar type)~~ Drop leap days based on CF calendar type to calculate daily climatologies and departures Sep 21, 2022

tomvothecoder commented Sep 21, 2022

View reviewed changes

tomvothecoder marked this pull request as ready for review September 21, 2022 22:48

tomvothecoder requested review from lee1043 and pochedls September 21, 2022 22:48

tomvothecoder added 5 commits September 21, 2022 15:50

Drop leap days to calculate daily climos and departs

e1f708a

Update API docs and notebook

98b4e2a

Update order of calendars in list

531b5ef

Fix markdown code formatting

298c7bc

tomvothecoder force-pushed the feature/337-climo-highfreq branch from 47ae66e to 298c7bc Compare September 21, 2022 22:50

lee1043 added 2 commits September 21, 2022 16:24

Changed section title Daily Cycle to Daily Climatology to avoid confu…

967f307

…sion with diurnal cycle

minor renaming of titles for daily climatology sections

3695697

lee1043 approved these changes Sep 21, 2022

View reviewed changes

tomvothecoder added 2 commits September 26, 2022 16:34

Update titles of sections in notebook

ac33f8b

Update date last updated in notebook

c8fe370

tomvothecoder merged commit 9bdd87c into main Sep 26, 2022

tomvothecoder deleted the feature/337-climo-highfreq branch September 26, 2022 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop leap days based on CF calendar type to calculate daily climatologies and departures #350

Drop leap days based on CF calendar type to calculate daily climatologies and departures #350

tomvothecoder commented Sep 21, 2022 •

edited

Loading

tomvothecoder left a comment •

edited

Loading

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

tomvothecoder Sep 21, 2022

codecov-commenter commented Sep 21, 2022 •

edited

Loading

lee1043 left a comment

		@@ -1382,6 +1383,97 @@ def test_weighted_daily_climatology(self):

		assert result.identical(expected)

		def test_weighted_daily_climatology_drops_leap_days_with_matching_calendar(self):

		@@ -1606,6 +1698,126 @@ def test_unweighted_seasonal_departures_with_JFD(self):

		assert result.identical(expected)

		def test_weighted_daily_departures_drops_leap_days_with_matching_calendar(self):

Drop leap days based on CF calendar type to calculate daily climatologies and departures #350

Drop leap days based on CF calendar type to calculate daily climatologies and departures #350

Conversation

tomvothecoder commented Sep 21, 2022 • edited Loading

Description

Checklist

tomvothecoder left a comment • edited Loading

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

tomvothecoder Sep 21, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 21, 2022 • edited Loading

Codecov Report

lee1043 left a comment

Choose a reason for hiding this comment

tomvothecoder commented Sep 21, 2022 •

edited

Loading

tomvothecoder left a comment •

edited

Loading

codecov-commenter commented Sep 21, 2022 •

edited

Loading