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 reduction and changes to Ensembles #63

Merged
merged 43 commits into from
Sep 15, 2022
Merged

Ensemble reduction and changes to Ensembles #63

merged 43 commits into from
Sep 15, 2022

Conversation

RondeauG
Copy link
Contributor

@RondeauG RondeauG commented Sep 7, 2022

Pull Request Checklist:

  • pre-commit hooks are installed/active in my local clone ($ pre-commit install)
  • This PR addresses an already opened issue (for bug fixes / features)
  • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • If a merge request has been made in parallel to this PR in xscen-notebooks, it is merged and the submodules have been updated.
  • HISTORY.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?

  • New file reduction.py that contains a function to help build the criteria data and an xclim.ensembles._reduce wrapper.

  • Changes to ensemble_stats to:

    • Support xclim.ensembles._robustness functions.
    • Support looping through multiple functions without having to call ensemble_stats every time.
    • Support weights.
  • The code that performs common_attrs_only has been moved to clean_up.

  • Removed the default to_level in clean_up.

  • New function generate_weights that uses metadata in Datasets to guess GCMs and RCMs and create weights accordingly.

  • New function unstack_id to reverse-engineer IDs.

  • generate_id now also accepts Datasets

Does this PR introduce a breaking change?

  • statistics / stats_kwargs have been changed/eliminated in ensemble_stats, respectively.

Other information:

@RondeauG
Copy link
Contributor Author

RondeauG commented Sep 7, 2022

Question 1:
I feel that some of the info / weights stuff I implemented in ensemble_stats could be moved in its own function, to make the code cleaner. Thoughts?

Question 2:
kmeans_reduce_ensemble has 3 outputs (selected, clusters, fig_data), while kkz_reduce_ensemble only has 1. Is there a clean way to address that?

xscen/ensembles.py Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Show resolved Hide resolved
xscen/utils.py Show resolved Hide resolved
xscen/utils.py Outdated Show resolved Hide resolved
xscen/utils.py Outdated Show resolved Hide resolved
RondeauG and others added 3 commits September 12, 2022 11:34
@RondeauG
Copy link
Contributor Author

RondeauG commented Sep 12, 2022

I added a couple checks before change_significance to make sure that the right data is sent to that function, since it assumes absolute deltas if ref==None. @aulemahal , this is probably something that could improved in xclim, as well as supporting Datasets in a similar way to what is done in ensembles._base.py

xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
RondeauG and others added 10 commits September 12, 2022 13:12
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Conflicts:
	xscen/ensembles.py
xscen/utils.py Outdated Show resolved Hide resolved
Co-authored-by: juliettelavoie <juliette.lavoie@hotmail.ca>
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

Beaucoup de commentaires qui ne sont au final que des affaires de styles, donc j'approuve et tu en fais ce que tu veux.

Je ne suis pas certain de voir ce qu'on pourrait porter dans xclim? Le check sur delta_kind ?

xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/ensembles.py Outdated Show resolved Hide resolved
xscen/reduce.py Outdated Show resolved Hide resolved
RondeauG and others added 2 commits September 15, 2022 09:25
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

J'ai quelques petits commentaires sur les notebooks, mais sinon c'est beau!

@RondeauG
Copy link
Contributor Author

Question 2:
kmeans_reduce_ensemble has 3 outputs (selected, clusters, fig_data), while kkz_reduce_ensemble only has 1. Is there a clean way to address that?

Je me rend compte qu'on n'a pas réglé cette question. Des idées ?

@juliettelavoie
Copy link
Contributor

Je pense que toujours retourner 3 trucs c'est ok.
Pour properties_and_measures, on retourne toujours 2 choses, mais si ref_for_measures n'est pas donné, le 2e output est un dataset vide. Peut-être que les 2 outputs optionnels peuvent être dans le bon type mais vide (eg. clusters= {})

xscen/utils.py Outdated Show resolved Hide resolved
xscen/utils.py Outdated Show resolved Hide resolved
@RondeauG RondeauG merged commit a3919b5 into main Sep 15, 2022
@RondeauG RondeauG deleted the reduce branch September 15, 2022 20:10
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.

Implement ensemble selection
4 participants