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

Fix indicator outputting delta degC #1973

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Fix indicator outputting delta degC #1973

merged 3 commits into from
Oct 24, 2024

Conversation

aulemahal
Copy link
Collaborator

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • 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?

Before this fix: an indicator that outputs "degC" but as a "temperature: difference" would bug. The compute function would return a dataset with proper units and proper "units_metadata" attribute and then the indicator would try to convert the data to the expected units, but doing so it would only give the units to convert_units_to. It would not give the units_metadata attribute.

thus, convert_units_to was taking the compute function output and getting "delta_degC", as expected, but then it was not able to convert to "degC", of course.

This change passes the whole attrs to convert_units_to so it can understand that the expected units are a difference.

Does this PR introduce a breaking change?

No.

We have no current indicator that output "degC" as "difference" (dtr outputs Kelvins), but the problem would emerge in custom indicators or with sdba.measures.bias where units are not explicitly set, so the "expected ones" are inferred from the compute function's output. Before this fix, the units_metadata attribute was dropped at that point, triggering the bug described above.

Other information:

@aulemahal aulemahal requested a review from huard October 24, 2024 18:59
@aulemahal aulemahal requested a review from Zeitsperre October 24, 2024 19:16
Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Approved for additional tests label Oct 24, 2024
@aulemahal aulemahal merged commit c4e7f8c into main Oct 24, 2024
30 checks passed
@aulemahal aulemahal deleted the fix-temp-diff-ind branch October 24, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants