-
Notifications
You must be signed in to change notification settings - Fork 59
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this 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 ; )
Co-authored-by: David Huard <[email protected]>
@huard After our discussions and some thinking, I rewrote the function completely. I was not satisfied with the output of Finally, I simplified the notebook a lot. May be too much... I'll add tests tomorrow. |
@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 |
@@ -164,6 +164,7 @@ | |||
"# Set display to HTML style (for fancy output)\n", |
There was a problem hiding this comment.
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
be5 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
There was a problem hiding this 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.
This looks pretty good! |
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
ipcc-advanced
as a test toensembles.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).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 :
change_significance
so thatpos_frac
is independent of change significancesignificant_change_frac, significant_positive_change_frac, positive_change_frac
.First two are breaking changes while the thirds feels overly heavy for something this simple...