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

Adapt to xclim 0.40 #248

Closed
wants to merge 4 commits into from

Conversation

aulemahal
Copy link
Contributor

@aulemahal aulemahal commented Dec 21, 2022

Pull Request to resolve #xxx

  • Unit tests cover the changes.
  • These changes were tested on real data.
  • The relevant documentation has been added or updated.
  • A short description of the changes has been added to doc/source/references/release_notes.rst.

Describe the changes you made

We will soon release xclim 0.40 in which there are some breaking changes for icclim:

  • One of them was brought by Multi-dimensional thresholds Ouranosinc/xclim#1236 where PercentileDataArray was deprecated. In this PR I simply ported the class to icclim.

  • Another is from Explicit context information for unit conversion Ouranosinc/xclim#1227 where the "hydro" unit context was deactivated by default. This was mainly used to convert between precipitation rate ( mm /d ) and flux (kg m-2 s-1). With additions from ENH: Update convert_units_to function Ouranosinc/xclim#1206, if the inputs of the convert_units_to function have valid and compatible standard names, the conversion can still be made automatically, but without these CF attributes, the default is to raise an error. This can be avoided by passing context='infer' or context='hydro' to convert_units_to. With the latter, the rate-to-flux transformations are always possible, while the former only activates them when the standard name of the input refers to water.

I saw the tests failures on my side, but I didn't want to apply a patch without opinions from people here.

@bzah
Copy link
Member

bzah commented Dec 22, 2022

Hi Pascal and thanks for the PR!

For the test failure I think it complains because at the moment icclim sees the 2 PercentileDataArray classes, one still in xclim 0.39 (percentile_doy returns an instance of it) and the other you just added.
It would probably be sufficient and future proof to modify the test with:

def test_build_percentile_threshold__from_file(self):
        doys = percentile_doy(self.data)
        doys = PercentileDataArray.from_da(doys) # <- Le fix
        doys.to_netcdf(path=self.IN_FILE_PATH)
        ...

As for the Hydro update, thanks for pointing it out, I didn't follow the discussion on that.
@pagecp should decide what should be the behavior of icclim, but I'm going to give my thoughts anyway:
I would be tempted to simply use infer because GenericIndicators make it impossible to know if we are in an hydro context or not.
We would also need to revert this commit so that the conversion logic is in one place (xclim).

@pagecp
Copy link
Collaborator

pagecp commented Jan 3, 2023

Hi Abel and all!
Yes I agree with Abel about the behavior of icclim!

@bzah
Copy link
Member

bzah commented Jan 11, 2023

The CI is broken when the PR comes from a fork and not icclim main repository, I tried to implement a fix but without success: #250
So as long as the unit tests are passing locally we should not block this PR from merging.

@bzah bzah mentioned this pull request Jan 16, 2023
7 tasks
@bzah
Copy link
Member

bzah commented Jan 16, 2023

Closing this pr in favor of the #251 to workaround the CI issue.
Beware, the branch Ouranosinc:adapt-to-xclim0-40 on Ouranosinc could become desynchronized

@bzah bzah closed this Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants