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

Explicit context information for unit conversion #1227

Merged
merged 14 commits into from
Dec 13, 2022
Merged

Explicit context information for unit conversion #1227

merged 14 commits into from
Dec 13, 2022

Conversation

huard
Copy link
Collaborator

@huard huard commented Nov 9, 2022

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

  • Disables hydro context
  • Enable hydro context for specific operations
  • Add infer_context function to encapsulate the logic determining context when not explicit (e.g. generic indices, datachecks, etc.)
  • Adjust tests

Does this PR introduce a breaking change?

Things that worked out of the box by assuming the hydro context applied to every xclim operation will break. A bit more care needs to be taken when writing indices, especially generic ones, to handle unit conversion correctly.

Other information:

…on] dimension. Added explicit unit conversion context.
@huard huard marked this pull request as draft November 9, 2022 16:59
@github-actions github-actions bot added the indicators Climate indices and indicators label Nov 9, 2022
@github-actions github-actions bot added docs Improvements to documenation sdba Issues concerning the sdba submodule. labels Nov 16, 2022
@huard huard marked this pull request as ready for review November 16, 2022 18:26
@huard huard requested a review from aulemahal November 28, 2022 14:05
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.

I find it non-ideal that the solution is as verbose as it is, but that's the current state of xclim's context management. And at least it is explicit.

I also added some conditions for infer_context, as well as a test for it!

@github-actions github-actions bot added the approved Approved for additional tests label Dec 5, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@aulemahal
Copy link
Collaborator

@huard I added the option to pass convert_units_to(src, tgt, context='infer') which will call infer_context on the standard names of either src or tgt. This allowed me to fix the remaining issues in sdba where we are dealing with dataarrays of an unknown unit. The default is still None.

@aulemahal
Copy link
Collaborator

@Zeitsperre It hung here too, so I simply commented it out.

@Zeitsperre
Copy link
Collaborator

This is really mysterious... I have a theory on what it could be but will explore more.

docs/notebooks/sdba.ipynb Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CI Automation and Contiunous Integration label Dec 12, 2022
@Zeitsperre Zeitsperre merged commit 9c295c5 into master Dec 13, 2022
@Zeitsperre Zeitsperre deleted the fix-1208 branch December 13, 2022 15:24
@aulemahal aulemahal mentioned this pull request Dec 13, 2022
5 tasks
@bzah bzah mentioned this pull request Jan 16, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests CI Automation and Contiunous Integration 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.

The "hydro" context is always on.
3 participants