Skip to content

Commit

Permalink
Remove columns that are all NaNs in make_criteria (#1713)
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:
- [x] 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] CHANGES.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?

* Methods such as kmeans clustering will crash if there are NaNs in the
data. This is something that should happen if only a subset of
realizations are NaNs, but we should be safe to remove the columns that
are NaN across all realizations.

### Does this PR introduce a breaking change?

* No


### Other information:
  • Loading branch information
RondeauG authored Apr 18, 2024
2 parents 6c1b71c + f44944b commit fb666c9
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 1 deletion.
3 changes: 2 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Changelog

v0.49.0 (unreleased)
--------------------
Contributors to this version: Trevor James Smith (:user:`Zeitsperre`), Pascal Bourgault (:user:`aulemahal`), Juliette Lavoie (:user:`juliettelavoie`), David Huard (:user:`huard`).
Contributors to this version: Trevor James Smith (:user:`Zeitsperre`), Pascal Bourgault (:user:`aulemahal`), Juliette Lavoie (:user:`juliettelavoie`), David Huard (:user:`huard`), Gabriel Rondeau-Genesse (:user:`RondeauG`).

Announcements
^^^^^^^^^^^^^
Expand All @@ -27,6 +27,7 @@ Bug fixes
* Fixed bug with loess smoothing for an array full of NaNs. (:pull:`1699`).
* Fixed and adapted ``time_bnds`` to the newest xarray. (:pull:`1700`).
* Fixed "agreement fraction" in ``robustness_fractions`` to distinguish between negative change and no change. Added "negative" and "changed negative" fractions (:issue:`1690`, :pull:`1711`).
* ``make_criteria`` now skips columns with NaNs across all realizations. (:pull:`1713`).

Internal changes
^^^^^^^^^^^^^^^^
Expand Down
9 changes: 9 additions & 0 deletions tests/test_ensembles.py
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,15 @@ def test_make_criteria(self, tas_series):
uncrit = crit.unstack("criteria")
assert set(uncrit.dims) == {"realization", "lat", "time"}

crit = ensembles.make_criteria(ds.where(ds.var_a > 0))
assert crit.dims == ("realization", "criteria")
assert crit.criteria.size == 12
np.testing.assert_array_equal(crit.isnull().sum(), 0)
np.testing.assert_array_equal(crit.min(), 1)
uncrit = crit.unstack("criteria").to_dataset("variables")
assert set(uncrit.dims) == {"realization", "lat", "time"}
assert uncrit.time.size == 3


# ## Tests for Robustness ##
@pytest.fixture
Expand Down
5 changes: 5 additions & 0 deletions xclim/ensembles/_reduce.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ def make_criteria(ds: xarray.Dataset | xarray.DataArray):
`ds2` will have all variables with the same dimensions, so if the original dataset had variables with different
dimensions, the added dimensions are filled with NaNs.
Also, note that criteria that are all NaN (such as lat/lon coordinates with no data) are dropped from `crit` to avoid issues with
the clustering algorithms, so the original dataset might not be able to be fully reconstructed.
The `to_dataset` part can be skipped if the original input was a DataArray.
"""

Expand Down Expand Up @@ -113,6 +115,9 @@ def _make_crit(da):
else:
# Easy peasy, skip all the convoluted stuff
crit = _make_crit(ds)

# drop criteria that are all NaN
crit = crit.dropna(dim="criteria", how="all")
return crit.rename("criteria")


Expand Down

0 comments on commit fb666c9

Please sign in to comment.