-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for temperature differences in CF attributes #1836
base: main
Are you sure you want to change the base?
Conversation
…nvert_units_to. Add test and docs.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@juliettelavoie @aulemahal Ce PR va possiblement affecter des fonctions que vous avez créées. |
out.attrs["units_metadata"] = "temperature: difference" | ||
return to_agg_units(out, data, "integral") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
…ng metadata to non temperature variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
CHANGELOG.rst
Outdated
* 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`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PVI, "pluvieux" avait été remplacé par "avec précipitation" à beaucoup d'endroits dans #1123.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, bon à savoir. Je vais faire un autre PR pour harmoniser.
xclim/sdba/properties.py
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
un ptit dernier!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…t for non-temperature variables.
Set
units_metadata
attribute to"temperature: difference"
for arrays representing a difference between two temperatures.Pull Request Checklist:
units_metadata
where appropriate. #1822number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
units_metadata
attribute set to"temperature: difference"
whenever values represent a difference between two temperatures (degree days, diurnal amplitude, bias, etc).delta_degC
, which is not CF compliant, by the original input DataArray units and theunits_metadata
attribute.pint2cfattrs
to convert delta units into a dictionary of attributes, includingunits_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.