Fix conversion of units of multivariate DataArray in sdba

coxipi commented 1 month ago

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.

coveralls commented 1 month ago

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

coxipi commented 1 month ago

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

Ouranosinc / xclim