Skip to content
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

Merged
merged 21 commits into from
Oct 17, 2024
Merged

Support for temperature differences in CF attributes #1836

merged 21 commits into from
Oct 17, 2024

Conversation

huard
Copy link
Collaborator

@huard huard commented Jul 11, 2024

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)
  • 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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added sdba Issues concerning the sdba submodule. docs Improvements to documenation labels Jul 11, 2024
@huard
Copy link
Collaborator Author

huard commented Jul 11, 2024

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

xclim/sdba/properties.py Outdated Show resolved Hide resolved
Copy link
Contributor

@juliettelavoie juliettelavoie left a 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 Show resolved Hide resolved
xclim/sdba/properties.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Jul 12, 2024
@coveralls
Copy link

coveralls commented Jul 12, 2024

Coverage Status

coverage: 89.356% (-0.1%) from 89.462%
when pulling 212725a on fix-1822
into 0a19979 on main.

@Zeitsperre Zeitsperre added this to the v0.53.0 milestone Oct 8, 2024
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good!

However, should we ensure that all indicators that give temperatures have the proper units_metadata? For example :

xc.atmos.daily_temperature_range(ds=ds)
# Attributes:
#    units:           K
#    units_metadata:  temperature: unknown

And while testing this I realized that:

xc.indices.daily_temperature_range(tasmin=tn, tasmax=tx)
#Attributes:
#    units:    delta_degC

Which isn't great as it's not CF. This problem predates your PR, but it would be nice to apply your changes to this indice!

@github-actions github-actions bot added the indicators Climate indices and indicators label Oct 8, 2024
@Zeitsperre Zeitsperre requested a review from aulemahal October 10, 2024 21:13
@Zeitsperre Zeitsperre removed this from the v0.53.0 milestone Oct 11, 2024
@huard huard merged commit b3acb71 into main Oct 17, 2024
20 of 21 checks passed
@huard huard deleted the fix-1822 branch October 17, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation indicators Climate indices and indicators sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use units_metadata where appropriate.
6 participants