Skip to content

Commit

Permalink
Generic season index - no season is length 0 (#1845)
Browse files Browse the repository at this point in the history
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [ ] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #xyz
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGELOG.rst has been updated (with summary of main changes)
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added

### What kind of change does this PR introduce?

* Implements `generic.season`, a generic index function for computing
the start/end/length of seasons. Based on `run_length.season` but with
units management and the comparison.
* Divide `run_length.season` into `season_start`, `season_end`, plus the
existing `season_length`. The idea of grouping code together was good,
but it added a lot of unnecessary computations when only one facet is
needed (i.e. most cases). I think I was able to divide it elegantly,
using only one magic fast path.
* Rewrite `frost_free_season_start` and `frost_free_season_end`. Those
were not respecting the "season" definition. Now they do.

### Does this PR introduce a breaking change?
`frost_free_season_start` and `frost_free_season_end` have changed.
Their notion of "season" is not more strict. I think the difference not
significative for most cases. No tests needed a change, but again our
tests are very simple.

All season length indicators will now return 0 when no season is found
(no start). Previously, when no start was found we returned 0 when an
end was found and `nan` otherwise. I think the idea was to assume that
neither end nor start meant the data was probably wrong. But that's the
job of the missing checks, not the `run_length` helper. Thus I applied
the more logical rule : no season == 0 length.

I have reverted some changes of #1796, I'm sorry. I'm trying to
generalize the indicators and it made more sense that all aspects of the
same "growing season" were using the same exact arguments.

### Other information:
This allows indices to use run length function that return a DataArray
after being called with a DataArray. This "type preservation" will be
quite handy in my next PR, stay tuned!

Sadly, this branch is based off #1838 because I need both for PC.


### Ugh
The amount of copied docstring text and signatures is ridiculous. I
really want to remove all those one-liner indices and only keep the
indicators. But I fear this would be too breaking of a change as it
would remove elements from the API.
Right now, we have inconsistent indicators with inconsistent
documentation because everything was copy-pasted by hand with
incremental changes that were not always ported back to the other
indices.
  • Loading branch information
aulemahal authored Aug 1, 2024
2 parents 1a5ddad + 986a6d8 commit ade41d7
Show file tree
Hide file tree
Showing 7 changed files with 484 additions and 302 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ Breaking changes
^^^^^^^^^^^^^^^^
* As updated in ``cf_xarray>=0.9.3``, dimensionless quantities now use the "1" units attribute as specified by the CF conventions, previously an empty string was returned. (:pull:`1814`).

Breaking changes
^^^^^^^^^^^^^^^^
* The definitions of the ``frost_free_season_start`` and ``frost_free_season_end`` have been slightly changed to be coherent with the ``frost_free_season_length`` and `xclim`'s notion of `season` in general. Indicator and indices signature have changed. (:pull:`1845`).
* Season length indicators have been modified to return ``0`` for all cases where a proper season was not found, but the data is valid. Previously, a ``nan`` was given if neither a start or an end were found, even if the data was valid, and a ``0`` was given if an end was found but no start. (:pull:`1845`).

Bug fixes
^^^^^^^^^
* Fixed the indexer bug in the ``xclim.indices.standardized_index_fit_params`` when multiple or non-array indexers are specified and fitted parameters are reloaded from netCDF. (:issue:`1842`, :pull:`1843`).
Expand All @@ -26,6 +31,7 @@ Internal changes
^^^^^^^^^^^^^^^^
* Changed the French translation of "wet days" from "jours mouillés" to "jours pluvieux". (:issue:`1825`, :pull:`1826`).
* In order to adapt to changes in `pytest`, the doctest fixtures have been split from the main testing suite and doctests are now run using ``$ python -c 'from xclim.testing.utils import run_doctests; run_doctests()'``. (:pull:`1632`).
* Added ``xclim.indices.generic.season`` to make season start, end, and length indices. Added a ``stat`` argument to ``xclim.indices.run_length.season`` to avoid returning a dataset. (:pull:`1845`).

CI changes
^^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion tests/test_indices.py
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ class TestGrowingSeasonLength:
[
("1950-01-01", "1951-01-01", 0), # No growing season
("2000-01-01", "2000-12-31", 365), # All year growing season
("2000-07-10", "2001-01-01", np.nan), # End happens before start
("2000-07-10", "2001-01-01", 0), # End happens before start
("2000-06-15", "2001-01-01", 199), # No end
("2000-06-15", "2000-07-15", 31), # Normal case
],
Expand Down
10 changes: 4 additions & 6 deletions tests/test_run_length.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ class TestRunsWithDates:
[
("07-01", 210, 70),
("07-01", 190, 50),
("04-01", 150, np.nan), # date falls early
("04-01", 150, 0), # date falls early
("11-01", 150, 165), # date ends late
(None, 150, 10), # no date, real length
],
Expand All @@ -492,7 +492,7 @@ def test_season_length(self, tas_series, date, end, expected, use_dask, ufunc):
runs,
window=1,
dim="time",
date=date,
mid_date=date,
)
np.testing.assert_array_equal(np.mean(out.load()), expected)

Expand Down Expand Up @@ -625,7 +625,7 @@ def test_run_with_dates_different_calendars(self, calendar, expected):
out = (
(tas > 0)
.resample(time="YS-MAR")
.map(rl.season_length, date="03-02", window=2)
.map(rl.season_length, mid_date="03-02", window=2)
)
np.testing.assert_array_equal(out.values[1:], [250, 250])

Expand All @@ -643,9 +643,7 @@ def test_run_with_dates_different_calendars(self, calendar, expected):
)
np.testing.assert_array_equal(out.values[1:], np.array(expected) + 1)

@pytest.mark.parametrize(
"func", [rl.first_run_after_date, rl.season_length, rl.run_end_after_date]
)
@pytest.mark.parametrize("func", [rl.first_run_after_date, rl.run_end_after_date])
def test_too_many_dates(self, func, tas_series):
tas = tas_series(np.zeros(730), start="2000-01-01")
with pytest.raises(ValueError, match="More than 1 instance of date"):
Expand Down
4 changes: 2 additions & 2 deletions tests/test_temperature.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ def test_real_data(self, atmosds):

class TestFrostSeasonLength:
def test_simple(self, tasmin_series):
a = np.zeros(730) + K2C + 15
a = np.zeros(731) + K2C + 15
a[300:400] = K2C - 5
a[404:407] = K2C - 5
tasmin = tasmin_series(a, start="2000-01-01")
Expand All @@ -380,7 +380,7 @@ def test_simple(self, tasmin_series):
np.testing.assert_array_equal(out, [np.nan, 100, np.nan])

out = atmos.frost_season_length(tasmin=tasmin, mid_date="07-01", freq="YS")
np.testing.assert_array_equal(out, [np.nan, np.nan])
np.testing.assert_array_equal(out, [0, 181])


class TestColdSpellDays:
Expand Down
Loading

0 comments on commit ade41d7

Please sign in to comment.