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 conversion of units of multivariate DataArray in sdba #1972

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Oct 23, 2024

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?

Conversion of units of multivariate DataArray is now properly handled sdba.TrainAdjust and sdba.Adjust. There was a bug where the units could be changed before a conversion of the magntitudes could occur.

e.g.

out = sdba.dOTC.adjust(sim=sim,ref=ref,hist=hist)

hist would be converted to ref units properly, and then, the units of sim would be changed too, without any actual conversion taking place. This may have something to do with the weirdness of Generators.

Does this PR introduce a breaking change?

No

Other information:

Just to show where I'm getting at, you can test:

from xclim import sdba
from xclim.testing import open_dataset
ds = open_dataset('sdba/ahccd_1950-2013.nc').isel(location=0)
ds2 = open_dataset('sdba/CanESM2_1950-2100.nc').isel(location=0)
da = sdba.stack_variables(ds)
da2 = sdba.stack_variables(ds2)
ref = da.sel(time=slice("1950", "1979"))
hist = da2.sel(time=slice("1950", "1979"))
sim = da2.sel(time=slice("1980", "2009"))
out = sdba.dOTC.adjust(sim=sim.copy(), ref=ref.copy(), hist=hist.copy()) 
print(out["multivar"].attrs["_units"], out.sel(multivar="tasmax").mean().values)

>>> ['mm day-1', 'degC'] 14.25703882896165  # new xclim
>>>['mm day-1', 'degC'] 282.0954876255738  # old xclim

You can see that scen_tasmax ~ 15 with the new xclim, as it should, it's reasonable temperature for degC, but with old xclim it's around scen_tasmax~282, clearly still magnitudes of K. This happens because internally, the units of sim were changed, but the values were not converted.

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Oct 23, 2024
@coxipi coxipi added approved Approved for additional tests bug Something isn't working labels Oct 23, 2024
@coxipi coxipi requested a review from aulemahal October 23, 2024 15:42
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 can't see exactly why the previous code didn't work, but this solution looks clean.

@coveralls
Copy link

Coverage Status

coverage: 89.377% (-0.01%) from 89.391%
when pulling 135630a on fix_multivariate_units_sdba
into df86b39 on main.

@coxipi
Copy link
Contributor Author

coxipi commented Oct 23, 2024

I can't see exactly why the previous code didn't work, but this solution looks clean.

Me neither, I'm glad you are surprised too haha

@coxipi coxipi merged commit aa52a71 into main Oct 23, 2024
37 of 38 checks passed
@coxipi coxipi deleted the fix_multivariate_units_sdba branch October 23, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests bug Something isn't working sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants