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

[sdba] Refactor interp_on_quantiles #941

Merged
merged 18 commits into from Dec 3, 2021
Merged

[sdba] Refactor interp_on_quantiles #941

merged 18 commits into from Dec 3, 2021

Conversation

aulemahal
Copy link
Collaborator

@aulemahal aulemahal commented Nov 29, 2021

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)
  • 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.
  • bumpversion patch has been called on this branch
  • The relevant author information has been added to .zenodo.json

What kind of change does this PR introduce?

I though the changes in #914 would address a bug I saw while computing ESPO-R. However, I had forgotten about the splitting pathways for group='time' vs group='time.prop'. That also meant testing was not sufficient. This PR changes xclim.sdba.utils.interp_on_quantiles so that:

  • All NaNs in the coordinates are dropped. Minor modification from the Extremes' PR, but dropping everything explicitly, instead of relying on interp1d, has the advantage that NaNs in the new coord are not considered "out of bounds", but NaNs. They return NaN in the output, not an extrapolated value.
  • All NaNs in the new coordinates are dropped. In the 2D case, with 'nearest', NaNs would cause QHull errors.

Does this PR introduce a breaking change?

  • Not really.

Other information:

The docstring was improved.

I realized that method='cubic' does not respect the constant extrapolation as set by extrapolate_qm... I added a note to the doc, but I'm not sure what to do here. @huard ? The logic we used works well with 'nearest' and 'linear', but as 'cubic' fetches a third point it estimates something else.

EDIT:
I compared the performance before and after the change and it is insignificant. griddata and interp1d are much slower than any numpy overhead I added, so I did not see any timing difference.

@aulemahal aulemahal requested a review from huard November 29, 2021 22:22
@coveralls
Copy link

coveralls commented Nov 29, 2021

Coverage Status

Coverage decreased (-0.08%) to 90.654% when pulling 68dd5c2 on sdba-fullskipna into 4267286 on master.

xclim/sdba/utils.py Outdated Show resolved Hide resolved
xclim/sdba/utils.py Outdated Show resolved Hide resolved
xclim/sdba/utils.py Outdated Show resolved Hide resolved
xclim/sdba/utils.py Outdated Show resolved Hide resolved
@aulemahal aulemahal changed the title [sdba] Skip all NaNs in interp_on_quantiles [sdba] Refactor interp_on_quantiles Dec 2, 2021
@aulemahal
Copy link
Collaborator Author

Last commits went deeper and refactored interp_on_quantiles.

  • Extrapolation is now handled by that function, without need to use extrapolate_qm. Changes to the adjustment methods are internal and nothing has changed in the API for the normal user.
  • However, extrapolation is now better handled. With interp='cubic', the constant extrapolation is fixed.
  • On a side note, I removed the interp argument of Grouper's init. It served almost no purpose, introducing more noise than doing good. It stays on Grouper.get_index as boolean only. It only matters with monthly grouping, so... almost never.
  • A bit of code refactor.

Should we remove extrapolate_qm? It is not used in the adjustment methods anymore.

@aulemahal aulemahal requested a review from huard December 2, 2021 22:15
Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

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

Does it make sense to repurpose test_extrapolate_qm to test the new extrapolation code ? I know its implicitly tested elsewhere, but still.

@aulemahal
Copy link
Collaborator Author

The new extrapolation code is explicitly tested in test_interp_on_quantiles! (see expe).

I would rather suggest to remove extrapolate_qm? I liked the way it dealt with the situation, but sadly griddata demands more complexity...

@huard
Copy link
Collaborator

huard commented Dec 3, 2021

All good then.
Ok for removal, I would be surprised anyone used that function directly. If we want to do it by the book, we could deprecate it for the next release before removal.

@aulemahal aulemahal merged commit f248b5c into master Dec 3, 2021
@aulemahal aulemahal deleted the sdba-fullskipna branch December 3, 2021 20:36
raquelalegre added a commit to UCL/xclim that referenced this pull request Dec 6, 2021
…/xclim into indices/potential_evapotranspiration

* 'indices/potential_evapotranspiration' of github.com:UCL/xclim:
  [sdba] Refactor interp_on_quantiles (Ouranosinc#941)
  Indices/heat index (Ouranosinc#915)
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.

None yet

4 participants