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

Ensemble agreement assigns members with exactly no change to the negative fraction #1690

Closed
2 tasks done
aulemahal opened this issue Mar 26, 2024 · 5 comments · Fixed by #1711
Closed
2 tasks done
Assignees
Labels
bug Something isn't working enhancement New feature or request standards / conventions Suggestions on ways forward

Comments

@aulemahal
Copy link
Collaborator

aulemahal commented Mar 26, 2024

Addressing a Problem?

robustness_fractions has the agree output which is the fraction of valid models agreeing on the sign of change. It is currently a simple calculation based on positive, fraction of valid members showing positive change.

In summary:

positive = delta > 0
agree = max(positive, 1 - positive)

If the delta has real zeros, they are not counted towards positive. A dataset where a few members showing real positive change could come out has having low agreement.

Potential Solution

I think it would be more intuitive if the agreement fraction included the members showing no change.

positive = delta > 0
negative = delta < 0
no_change = delta == 0
agree = max(positive + no_change, negative + no_change)

This is also the opinion of the Portraits Climatiques team.

Additional context

The previous value of agree could always be recomputed by a user as positive is an output of the function.

What does @RondeauG and @huard think of this ?

Contribution

  • I would be willing/able to open a Pull Request to contribute this feature.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal aulemahal added enhancement New feature or request standards / conventions Suggestions on ways forward labels Mar 26, 2024
@aulemahal aulemahal added this to the Summer 2024 milestone Mar 26, 2024
@aulemahal aulemahal self-assigned this Mar 26, 2024
@huard
Copy link
Collaborator

huard commented Mar 26, 2024

I would tend to disagree. I consider a model with a change of 2C in disagreement with a model with a change of 0C.

I understand this is beyond the scope of this function, but in my mind you'd have a threshold t such that models can be divided into

negative change < -t <= no significant change <= t < positive change

Here t is 0, and values with 0 change would simply be part of another category of "no change".

My suggestion is thus to say:

positive = delta > 0
negative = delta < 0
agree = max(positive, negative)

@aulemahal
Copy link
Collaborator Author

aulemahal commented Mar 26, 2024

I understand. The threshold idea is not really beyond the scope of the function, ... it is pretty much what the threshold method does!

In fact, I can indeed avoid my problem by using that method, I didn't think of that! Thanks!

@RondeauG
Copy link
Contributor

RondeauG commented Apr 3, 2024

Let's say that we have 15 simulations: 5 negative, 4 not significantly negative, 3 with a change of exactly +0, 2 not significantly positive, 1 positive.

  • changed: Unless test=None --> 6/15.
  • positive: I'd argue that exactly +0 is not positive, thus positive = delta > 0 is correct. --> 3/15.
  • changed_positive: A change of +0 will not be counted here, since it will fail both tests. --> 1/15.
  • agree: max(3/15, 1 - 3/15) = 12/15 --> This is wrong! --> This should be max(positive, negative), aka 9/15.

@aulemahal
Copy link
Collaborator Author

The scientific consensus has spoken against my idea.
Thanks!

@aulemahal aulemahal closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2024
@aulemahal
Copy link
Collaborator Author

Reopen as our agree is indeed incorrect if there is an ambiguity between negative and no change.

Best solution to date:

positive = delta > 0
negative = delta < 0
agree = max(positive, negative, 1 - positive - negative)

With a possibility of most models "agreeing" on exactly no change.

@aulemahal aulemahal reopened this Apr 3, 2024
@aulemahal aulemahal changed the title Ensemble agreement does not include members with exactly no change Ensemble agreement assigns members with exactly no change to the negative fraction Apr 3, 2024
@Zeitsperre Zeitsperre added the bug Something isn't working label Apr 16, 2024
aulemahal added a commit that referenced this issue Apr 17, 2024
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #1690 
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGES.rst has been updated (with summary of main changes)
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added

### What kind of change does this PR introduce?
In `robustness_fractions`

* Computes the fraction of negative change explicitly and returns it 
* The agreement fraction is the maximum of the positive, negative or no
change fractions.

### Does this PR introduce a breaking change?
Yes, `agree_frac` has changed. However, it now better reflects its
definition and usual expectations. And the case where "no change" is the
largest group should not be very frequent, it usually happens with
zero-bounded indicators.


### Other information:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request standards / conventions Suggestions on ways forward
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants