Support for temperature differences in CF attributes #1836

huard · 2024-07-11T14:32:17Z

Set units_metadata attribute to "temperature: difference" for arrays representing a difference between two temperatures.

Pull Request Checklist:

This PR addresses an already opened issue (for bug fixes / features)
- This PR fixes Use units_metadata where appropriate. #1822
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?

Add units_metadata attribute set to "temperature: difference" whenever values represent a difference between two temperatures (degree days, diurnal amplitude, bias, etc).
Adjust the units handling functions to recognize this attribute.
Modify indices that set units to delta_degC, which is not CF compliant, by the original input DataArray units and the units_metadata attribute.
Create new function pint2cfattrs to convert delta units into a dictionary of attributes, including units_metadata.

Does this PR introduce a breaking change?

Yes, units returned by some indices will change.

Other information:

I suspect I missed some instances where fixes are needed.

…nvert_units_to. Add test and docs.

review-notebook-app · 2024-07-11T14:32:23Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

huard · 2024-07-11T14:38:38Z

@juliettelavoie @aulemahal Ce PR va possiblement affecter des fonctions que vous avez créées.

aulemahal · 2024-07-11T15:17:44Z

xclim/indices/generic.py

+    out.attrs["units_metadata"] = "temperature: difference"
    return to_agg_units(out, data, "integral")


For degC input, this outputs:

units_metadata: temperature: difference units: K d

because #1804 decided that we should avoid delta temperatures and put absolute ones instead.

Should we revert this ? In any case, I think to_agg_units could be the one to add units_metadata.

You mean revert going through absolute temperature ? I think this is not necessary with this PR.

Not sure about to_agg_units. Is your logic "if we aggregate temperatures, then it's obviously a difference" ?

Well, going to absolute temperatures and this PR both try to solve the same problem. I guess they are compatible, but I feel both aren't needed at the same time. We could revert to make it cleaner.

My point about to_agg_units is that that function is there to manage units after a transformation. Instead of going per-indicator, you could simply infer the "units_metadata" needed for the given transformation. Like I did in #1804.

I didn't want to sabotage the work you had done and it seemed prudent to keep the changes as small as possible. So no strong opinion on this one.

My thinking was that to_agg_units has no way to know whether it is aggregating a temperature or a temperature difference if we give it a DataArray with units of K.

Well it now does/could with your additions, no ? But in any case, isn't the logic the same ? All operations that transform an on_scale to a difference will also transform a difference to a difference. I don't think we have operations that would actually go from a difference to an on_scale ?

xclim/sdba/properties.py

…ng metadata to non temperature variables

xclim/sdba/measures.py

…adata attribute

juliettelavoie

lgtm!

juliettelavoie · 2024-07-12T13:02:54Z

CHANGELOG.rst

-* Changed french translation of "wet days" from "jours mouillés" to "jours pluvieux". (:issue:`1825`, :pull:`1826`).
-
+* Add attribute ``units_metadata`` to outputs representing a difference between temperatures. This is needed to disambiguate temperature differences from absolute temperature, and affects indicators using functions ``cumulative_difference``, ``temperature_sum``, ``interday_diurnal_temperature_range`` and `` extreme_temperature_range``.  Added ``pint2cfattrs`` function to convert pint units to a dictionary of CF attributes. ``units2pint`` is also modified to support ``units_metadata`` attributes in DataArrays. Some SDBA properties and measures previously returning units of ``delta_degC`` will now return the original input DataArray units accompanied with the ``units_metadata`` attribute. (:issue:`1822`, :pull:`1830`).
+* Changed french translation of "wet days" from "jours mouillés" to "jours pluvieux". (:issue:`1825`, :pull:`1836`).


PVI, "pluvieux" avait été remplacé par "avec précipitation" à beaucoup d'endroits dans #1123.

Ok, bon à savoir. Je vais faire un autre PR pour harmoniser.

juliettelavoie · 2024-07-12T13:12:42Z

xclim/sdba/properties.py

@@ -699,7 +699,8 @@ def _annual_statistic(

    if stat == "absamp":
        out = yrs.max() - yrs.min()
-        out.attrs["units"] = xc.core.units.ensure_delta(units)
+        out.attrs["units"] = units
+        out.attrs["units_metadata"] = "temperature: difference"


un ptit dernier!

Merci ! Ça me fait penser que stat == "relamp" devrait probablement appeler ensure_absolute_units, sinon ça pourrait donner des choses bizarres (genre division par 0 degC).

Je ne trouve pas ensure_absolute_units, juste ensure_absolute_temperature. Cette fonction assume un delta. ça ne fonctionne donc pas vrm ici pour empêcher une division par 0. Je pourrais ajouter un test spécifiquement pour modifier les unités du input si c'est une température, mais comme c'est une fonction généraliste, ça n'empêche pas quelqu'un de l'utiliser avec une autre variable ayant un 0. J'ai l'impression que c'est la responsabilité de l'utilisateur de mettre les bonnes unités avec relamp...

En théorie il n'y a pas d'autres unités ayant le problème "difference" vs "on_scale". En tout cas, rien dans pint, rien dans CF.

mais le problème de division par zero dans "relamp" ne dépend pas de "difference" vs "on_scale".
Et admettons qu'il n'y ai pas de 0, je ne vois pas le problème de faire (deg C-degC)/degC = %. pas besoin de spécifier "difference" ou "on_scale" dans le processus, on est sur que c'est toujours la même unité, non ?

En effet, les unités de relamp ne dépendent pas de rien d'autre. Et je suis d'accord que la division par zéro est un autre problème.

Même que de changer les unités de degC a K pour calculer un % n'est peut-être pas quelque chose qu'on devrait faire silencieusement ? Je propose de pelleter le problème chez l'usager. Et donc de ne rien faire.

coveralls · 2024-07-12T13:24:53Z

coverage: 90.613% (-0.05%) from 90.658%
when pulling 5539c79 on fix-1822
into 583f99c on main.

…t for non-temperature variables.

xclim/core/units.py

Implement pint2cfattrs. Handle units_metadata in units2pint. Adapt co…

66ce334

…nvert_units_to. Add test and docs.

github-actions bot added sdba Issues concerning the sdba submodule. docs Improvements to documenation labels Jul 11, 2024

adjust PR #

a403c22

huard requested review from aulemahal and juliettelavoie July 11, 2024 14:37

aulemahal reviewed Jul 11, 2024

View reviewed changes

xclim/sdba/properties.py Outdated Show resolved Hide resolved

juliettelavoie mentioned this pull request Jul 11, 2024

cf conventions for dtr Ouranosinc/xscen#428

Open

Fix for Indicator class. Use pint2cfattrs more broadly to avoid setti…

b93ba8a

…ng metadata to non temperature variables

juliettelavoie reviewed Jul 11, 2024

View reviewed changes

xclim/sdba/measures.py Outdated Show resolved Hide resolved

huard added 4 commits July 11, 2024 13:18

fix SDBA measures

6227609

add tests

2d59a8d

Merge branch 'main' into fix-1822

4fae706

include count in operations whose units should not have a units_met…

f891332

…adata attribute

juliettelavoie approved these changes Jul 12, 2024

View reviewed changes

github-actions bot added the approved Approved for additional tests label Jul 12, 2024

use pint2cfattributes instead of hard-coding units_metadata to accoun…

5539c79

…t for non-temperature variables.

VascoSch92 reviewed Jul 14, 2024

View reviewed changes

xclim/core/units.py Show resolved Hide resolved

Merge branch 'main' into fix-1822

9083982

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for temperature differences in CF attributes #1836

Support for temperature differences in CF attributes #1836

huard commented Jul 11, 2024 •

edited by Zeitsperre

Loading

review-notebook-app bot commented Jul 11, 2024

huard commented Jul 11, 2024

aulemahal Jul 11, 2024 •

edited

Loading

huard Jul 11, 2024

aulemahal Jul 11, 2024

huard Jul 11, 2024

aulemahal Jul 11, 2024

juliettelavoie left a comment

juliettelavoie Jul 12, 2024 •

edited

Loading

huard Jul 12, 2024

juliettelavoie Jul 12, 2024

huard Jul 12, 2024

juliettelavoie Jul 16, 2024

aulemahal Jul 16, 2024

juliettelavoie Jul 16, 2024

aulemahal Jul 16, 2024 •

edited

Loading

coveralls commented Jul 12, 2024 •

edited

Loading

		out.attrs["units_metadata"] = "temperature: difference"
		return to_agg_units(out, data, "integral")

Support for temperature differences in CF attributes #1836

Are you sure you want to change the base?

Support for temperature differences in CF attributes #1836

Conversation

huard commented Jul 11, 2024 • edited by Zeitsperre Loading

Pull Request Checklist:

What kind of change does this PR introduce?

Does this PR introduce a breaking change?

Other information:

review-notebook-app bot commented Jul 11, 2024

huard commented Jul 11, 2024

aulemahal Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliettelavoie left a comment

Choose a reason for hiding this comment

juliettelavoie Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulemahal Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 12, 2024 • edited Loading

huard commented Jul 11, 2024 •

edited by Zeitsperre

Loading

aulemahal Jul 11, 2024 •

edited

Loading

juliettelavoie Jul 12, 2024 •

edited

Loading

aulemahal Jul 16, 2024 •

edited

Loading

coveralls commented Jul 12, 2024 •

edited

Loading