Dynamic time selection

[baptistehamon]

Pull Request Checklist:

• This PR addresses an already opened issue (for bug fixes / features)
  • This PR fixes Dynamic time subset selection #1987
• Tests for the changes have been added (for bug fixes / features)
  • (If applicable) Documentation has been added / updated (for bug fixes / features)
• CHANGELOG.rst has been updated (with summary of main changes)
  • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

The PR improves the select_time function providing the possibility to select time based on xr.DataArray of days of year.

• Move _get_doys outside select_time
• Create mask_between_doys function
• Edit select_time to work with the new workflow to select time with doys

Other information:

I've run pytest but I had an error (ERROR tests/test_sdba/test_adjustment.py::TestLoci::test_reduce_dims) and a failed (FAILED tests/test_modules.py::test_encoding - AttributeError: module '_locale' has no attribute 'nl_langinfo') but I don't know how to fix this.

[github-actions]

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

• The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

[github-actions]

Warning

This Pull Request is coming from a fork and must be manually tagged approved in order to perform additional testing.

[Zeitsperre]

@baptistehamon Thanks so much for the Pull Request!

I'm curious about the error and failure you ran into. Do you think you could post the stack trace of those problems, either in a comment here or in a gist?

The nl_langinfo failure is unique to Windows and seems to not be covered in our CI testing ('slow' tests aren't run on Windows atm). Will see what can be done to address this in another PR.

[baptistehamon]

Here is the stack trace when I run $ pytest:

============================= test session starts =============================
platform win32 -- Python 3.12.7, pytest-7.4.4, pluggy-1.5.0
rootdir: P:\My Documents\GitHub\GitHub-Contribs\xclim
configfile: pyproject.toml
testpaths: tests
plugins: anyio-4.6.2.post1, xdist-3.6.1, xdoctest-1.2.0
collected 1713 items

tests\test_analog.py ......................... [ 1%]
tests\test_atmos.py ............................. [ 3%]
tests\test_bootstrapping.py ................................. [ 5%]
tests\test_calendar.py ................................................. [ 7%]
............ [ 8%]
tests\test_cffwis.py ............................ [ 10%]
tests\test_checks.py ........................... [ 11%]
tests\test_cli.py .......................... [ 13%]
tests\test_ensembles.py ........................................... [ 15%]
tests\test_ffdi.py .................... [ 17%]
tests\test_flags.py ......... [ 17%]
tests\test_formatting.py ... [ 17%]
tests\test_generic.py .................................................. [ 20%]
..................... [ 21%]
tests\test_generic_indicators.py ............... [ 22%]
tests\test_helpers.py .............. [ 23%]
tests\test_hydrology.py ......... [ 24%]
tests\test_indicators.py ........................................ [ 26%]
tests\test_indices.py .................................................. [ 29%]
........................................................................ [ 33%]
........................................................................ [ 37%]
...........................s..s......................................... [ 41%]
........................................................................ [ 46%]
........................................................................ [ 50%]
.. [ 50%]
tests\test_land.py .......... [ 51%]
tests\test_locales.py ......... [ 51%]
tests\test_missing.py .............................. [ 53%]
tests\test_modules.py .....................sssssssssssssss.ss...s.s.s... [ 56%]
.s..........................ss..ss..ss..s.ss......................ssssss [ 60%]
....ssss........F [ 61%]
tests\test_options.py .................... [ 62%]
tests\test_partitioning.py ..... [ 62%]
tests\test_precip.py ................................. [ 64%]
tests\test_preciptemp.py .... [ 65%]
tests\test_run_length.py ....s.s........................................ [ 67%]
........................................................................ [ 72%]
........................................................................ [ 76%]
..................... [ 77%]
tests\test_seaice.py ........ [ 77%]
tests\test_snow.py ............. [ 78%]
tests\test_stats.py .................................................. [ 81%]
tests\test_temperature.py .............................................. [ 84%]
................................... [ 86%]
tests\test_testing_utils.py ....... [ 86%]
tests\test_units.py ................................................ [ 89%]
tests\test_utils.py ........ [ 90%]
tests\test_wind.py ....... [ 90%]
tests\test_sdba\test_adjustment.py .......E.......XXXX.................. [ 92%]
.....................ssssss..s [ 94%]
tests\test_sdba\test_base.py ................... [ 95%]
tests\test_sdba\test_detrending.py ..... [ 95%]
tests\test_sdba\test_loess.py ......... [ 96%]
tests\test_sdba\test_measures.py ......... [ 96%]
tests\test_sdba\test_nbutils.py ... [ 96%]
tests\test_sdba\test_processing.py ............... [ 97%]
tests\test_sdba\test_properties.py .................... [ 99%]
tests\test_sdba\test_sdba_utils.py ................. [100%]

=================================== ERRORS ====================================
_________________ ERROR at setup of TestLoci.test_reduce_dims _________________
file P:\My Documents\GitHub\GitHub-Contribs\xclim\tests\test_sdba\test_adjustment.py, line 133
@pytest.mark.requires_internet
def test_reduce_dims(self, ref_hist_sim_tuto):
file P:\My Documents\GitHub\GitHub-Contribs\xclim\tests\test_sdba\conftest.py, line 107
@pytest.fixture
def ref_hist_sim_tuto(socket_enabled): # noqa: F841
E fixture 'socket_enabled' not found

  available fixtures: anyio_backend, anyio_backend_name, anyio_backend_options, areacella, areacello, atmosds, cache, cannon_2015_dist, cannon_2015_rvs, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, ensemble_dataset_objects, evspsbl_series, evspsblpot_series, gather_session_data, hurs_series, huss_series, lafferty_sriver_ds, lat_series, make_qm, mon_series, mon_triangular, monkeypatch, ndq_series, nimbus, official_indicators, open_dataset, per_doy, pr_hr_series, pr_ndseries, pr_series, prc_series, prsn_series, prsnd_series, ps_series, pytestconfig, q_series, qds_month, qm_month, qm_small, random, record_property, record_testsuite_property, record_xml_attribute, recwarn, ref_hist_sim_tuto, rlds_series, rlus_series, rsds_series, rsus_series, series, sfcWind_series, sfcWindmax_series, snd_series, snw_series, tas_series, tasmax_series, tasmin_series, testrun_uid, threadsafe_data_dir, timeseries, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory, worker_id, xdoctest_namespace
  use 'pytest --fixtures [testpath]' for help on them.

P:\My Documents\GitHub\GitHub-Contribs\xclim\tests\test_sdba\conftest.py:107
================================== FAILURES ===================================
________________________________ test_encoding ________________________________

@pytest.mark.slow
def test_encoding():
    import _locale
    import sys

    # remove xclim
    del sys.modules["xclim"]

    # patch so that the default encoding is not UTF-8

  old = _locale.nl_langinfo

E AttributeError: module '_locale' has no attribute 'nl_langinfo'

tests\test_modules.py:286: AttributeError
============================== warnings summary ===============================
tests/test_cli.py::test_normal_computation[tg_mean-272.15-varnames0]
:488: RuntimeWarning: numpy.ndarray size changed, may indicate binary incompatibility. Expected 16 from C header, got 96 from PyObject

tests/test_generic_indicators.py::TestFit::test_nan
tests/test_precip.py::TestSnowfallDate::test_first_snowfall
tests/test_precip.py::TestSnowfallDate::test_last_snowfall
tests/test_precip.py::TestSnowfallMeteoSwiss::test_snowfall_frequency
tests/test_precip.py::TestSnowfallMeteoSwiss::test_snowfall_intensity
tests/test_temperature.py::test_degree_days_exceedance_date
P:\My Documents\GitHub\GitHub-Contribs\xclim\src\xclim\core\indicator.py:1580: DeprecationWarning: Bitwise inversion '~' on bool is deprecated and will be removed in Python 3.16. This returns the bitwise inversion of the underlying int object and is usually not what you expect from negating a bool. Use the 'not' operator for boolean negation or ~int(x) if you really want the bitwise inversion of the underlying int.
outs = [out.where(~mask) for out in outs]

tests/test_indices.py: 12 warnings
tests/test_sdba/test_base.py: 24 warnings
C:\Users\bha170\AppData\Local\miniconda3\envs\xclim\Lib\site-packages\xarray\core\dataset.py:227: FutureWarning: dt.weekofyear and dt.week have been deprecated. Please use dt.isocalendar().week instead.
data = getattr(ref_var.dt, var_name).data

tests/test_units.py::test_to_agg_units[\xb0C-argmax-doymax-0-1]
C:\Users\bha170\AppData\Local\miniconda3\envs\xclim\Lib\site-packages\xarray\core\dataarray.py:6293: DeprecationWarning: Behaviour of argmin/argmax with neither dim nor axis argument will change to return a dict of indices of each dimension. To get a single, flat index, please use np.argmin(da.data) or np.argmax(da.data) instead of da.argmin() or da.argmax().
result = self.variable.argmax(dim, axis, keep_attrs, skipna)

tests/test_sdba/test_adjustment.py::TestExtremeValues::test_nan_values
P:\My Documents\GitHub\GitHub-Contribs\xclim\src\xclim\sdba\utils.py:356: RuntimeWarning: All-nan slice encountered in interp_on_quantiles
warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ===========================
SKIPPED [2] tests\test_indices.py:2457: Daily input freq and dask arrays not working
SKIPPED [1] tests\test_modules.py:58: Indicator cf.cddcoldTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.cfd has no default for constrain.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.csu has no default for constrain.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctmgeTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctmgtTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctmleTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctmltTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctngeTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctngtTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctnleTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctnltTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctxgeTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctxgtTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctxleTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ctxltTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ddgtTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.ddltTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.fxx has no default for wsgsmax.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.gddgrowTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.hddheatTT has no default for threshold.
SKIPPED [1] tests\test_modules.py:58: Indicator cf.sdii has no default for constrain.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TG90p has no default for tas_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TG10p has no default for tas_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TX90p has no default for tasmax_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TX10p has no default for tasmax_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TN90p has no default for tasmin_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.TN10p has no default for tasmin_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.HI has no default for lat.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.CSDI has no default for tasmin_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.WSDI has no default for tasmax_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R75p has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R95p has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R99p has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R75pTOT has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R95pTOT has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.R99pTOT has no default for pr_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.CD has no default for tas_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.WD has no default for tas_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.WW has no default for tas_per.
SKIPPED [1] tests\test_modules.py:58: Indicator icclim.CW has no default for tas_per.
SKIPPED [2] tests\test_run_length.py:98: rle_1d is not implemented for dask arrays.
SKIPPED [1] tests\test_sdba\test_adjustment.py:866: could not import 'ot': No module named 'ot'
SKIPPED [4] tests\test_sdba\test_adjustment.py:921: could not import 'ot': No module named 'ot'
SKIPPED [1] tests\test_sdba\test_adjustment.py:1002: could not import 'ot': No module named 'ot'
SKIPPED [1] tests\test_sdba\test_adjustment.py:1044: got empty parameter set ['method'], function test_sbck at P:\My Documents\GitHub\GitHub-Contribs\xclim\tests\test_sdba\test_adjustment.py:1043
XPASS tests/test_sdba/test_adjustment.py::TestDQM::test_mon_U[True-+-tas] This test sometimes fails due to a block/indexing error
XPASS tests/test_sdba/test_adjustment.py::TestDQM::test_mon_U[True--pr] This test sometimes fails due to a block/indexing error
XPASS tests/test_sdba/test_adjustment.py::TestDQM::test_mon_U[False-+-tas] This test sometimes fails due to a block/indexing error
XPASS tests/test_sdba/test_adjustment.py::TestDQM::test_mon_U[False--pr] This test sometimes fails due to a block/indexing error
ERROR tests/test_sdba/test_adjustment.py::TestLoci::test_reduce_dims
FAILED tests/test_modules.py::test_encoding - AttributeError: module '_locale' has no attribute 'nl_langinfo'
= 1 failed, 1656 passed, 51 skipped, 4 xpassed, 45 warnings, 1 error in 587.98s (0:09:47) =

[aulemahal]

@baptistehamon Sorry for the delay, last days were busy at Ouranos.

I took the liberty to edit your code, I hope you don't mind too much... My first goal was to add support to the case where the doy bounds are DataArrays with non-temporal dimensions, and I got carried away. The logic for the "time" case is pretty much the same though. I tried to simplify where I could and use purer xarray syntax to cover more cases.

I'll write a summary of the major changes I made tomorrow.

[baptistehamon]

I haven't thought about cases without a 'time' dimension, thanks a lot for implementing and for improving my PR !

[aulemahal]

Woops, when I wrote my last comment, I forgot that it was friday. So by « tomorrow » I meant « the next working day » ;).

My changes:

• transform a NaN value into an open bound (instead of invalidating the period)
• are a little stricter as start and end must have the exact same time coordinate (if they have one) (same calendea, same timesteps). A calendar different than da is still supported.
• support non-temporal dims on the bounds, again with the same restriction : shared dims must be exactly the same.

I'll add a few tests for the new cases when I get more time to work on this!

[aulemahal]

Ok, so the last commit finished up the work on select_time and added tests for that function. However, I am now stuck on how to make the missing checks work with the case where bounds are given on non-temporal dimensions... The missing check, even with the latest PR, make use of the drop=True argument of select_time and of course, that doesn't work as expected if the selection varies on a dimension...

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[aulemahal]

Ouf. That required more change than I first thought!

@huard Most notably, this PR makes three significant changes to the missing methods:

• The indexing within the missing methods doesn't drop=True anymore. With non-scalar bounds like what this PR adds, it is not possible to drop all masked values anymore and as such we can't use the count of timesteps in the missing checks.
• Because of the latter, I rewrote almost all missing methods and it made more sense to invert the logic : is_missing receives valid the array of valid values (instead of null).

The results are now a bit different than before as one can see with the test_missing tests I removed. Mostly, because of the previous drop=True the time coordinate of the returned missing values mask would sometimes be shorter than the one expected by a pure da.resample(time=freq). I would argue that this was not good. I think the missing_XXX function should return data on the same dimensions as the indicator itself. And I would argue that our doc was not making it clear that the previous behaviour was possible, even though the tests were clearly written to test it.

The tests I added are quite simple, but they should cover most cases of normal use.

@baptistehamon thanks for starting this PR, even if it took me more work, it accelerated long-needed changes

[huard]

I read through the code and could mostly understand what's going on. The only thing that I believe is missing from the testing is the include_bounds logic. It's simple enough that I assume its working, but still.