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

Refactor robustness methods #1514

Merged
merged 12 commits into from Nov 6, 2023
Merged

Refactor robustness methods #1514

merged 12 commits into from Nov 6, 2023

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)
  • CHANGES.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?

  • Add ipcc-advanced as a test to ensembles.change_significance. This corresponds to the simpler alternative of approach C as described in the Cross-Chapter Box 1 of the IPCC Atlas (AR6, WG1).
  • Add two examples in the "Ensembles" notebook to show how xclim can be used to get the IPCC-recommended hatching masks.
  • Add a very simple detrend function.

Does this PR introduce a breaking change?

No, but it might, see below.

Other information:

As you can see in the examples, the IPCC decouples the "change significance" from the "sign agreement", while xclim does not. This make the second example more complex than expected.

I thought of :

  • Changing the output of change_significance so that pos_frac is independent of change significance
  • Adding a third output so we have: significant_change_frac, significant_positive_change_frac, positive_change_frac.
  • Adding a keyword argument to switch between the two modes.
    First two are breaking changes while the thirds feels overly heavy for something this simple...

@aulemahal aulemahal added the help wanted Extra attention is needed label Oct 27, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the docs Improvements to documenation label Oct 27, 2023
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.

  • Looks good.
  • The nb text assumes the reader's familiarity with the topic and the IPCC approach. I suggest to take a more pedagogic approach.
  • I don't see a test ; )

docs/notebooks/ensembles.ipynb Outdated Show resolved Hide resolved
docs/notebooks/ensembles.ipynb Outdated Show resolved Hide resolved
docs/notebooks/ensembles.ipynb Outdated Show resolved Hide resolved
docs/notebooks/ensembles.ipynb Outdated Show resolved Hide resolved
docs/notebooks/ensembles.ipynb Outdated Show resolved Hide resolved
xclim/ensembles/_robustness.py Outdated Show resolved Hide resolved
xclim/ensembles/_robustness.py Outdated Show resolved Hide resolved
xclim/ensembles/_robustness.py Outdated Show resolved Hide resolved
@aulemahal
Copy link
Collaborator Author

@huard After our discussions and some thinking, I rewrote the function completely.

I was not satisfied with the output of change_significance so I rewrote it as robustness_fractions. The former still works but it will warn the user that it is deprecated. Also added robustness_categories to help with making the actual map.

Finally, I simplified the notebook a lot. May be too much...

I'll add tests tomorrow.

@aulemahal
Copy link
Collaborator Author

@RondeauG , would you have time to look at the new functions ? And may be @mccrayc, if you have time.

I'm simply looking for a review of the general idea and usage, as shown in the example : https://xclim--1514.org.readthedocs.build/en/1514/notebooks/ensembles.html#Change-significance-and-model-agreement

@aulemahal aulemahal changed the title Add IPCC robustness method to change_significance Refactor robustness methods Nov 3, 2023
@coveralls
Copy link

coveralls commented Nov 3, 2023

Coverage Status

coverage: 90.435%. first build
when pulling 930c272 on ipcc-robustness
into 3406661 on master.

@@ -164,6 +164,7 @@
"# Set display to HTML style (for fancy output)\n",
Copy link
Contributor

@RondeauG RondeauG Nov 3, 2023

Choose a reason for hiding this comment

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

Quelques détails de syntaxe ou typo:

  • surimposed --> superimposed
  • change significance : whether most of the ensemble members project a statistically significant climate change signal, in comparison to their internal variability.
  • There are be 5 different members in the ensemble

(models agree and all show significant change)

Je dirais plutôt enough show a significant change. L'IPCC utilise un seuil de X% (60?).


Reply via ReviewNB

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.

Ça me semble beaucoup plus clair comme ça. Pas testé en vrai, juste lu en diagonale.

@github-actions github-actions bot added the approved Approved for additional tests label Nov 3, 2023
@RondeauG
Copy link
Contributor

RondeauG commented Nov 3, 2023

This looks pretty good! robustness_categories will be very useful.

@aulemahal aulemahal merged commit 19b3cad into master Nov 6, 2023
16 checks passed
@aulemahal aulemahal deleted the ipcc-robustness branch November 6, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants