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

add get_partition_input #289

Merged
merged 52 commits into from
Jan 10, 2024
Merged

add get_partition_input #289

merged 52 commits into from
Jan 10, 2024

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Nov 17, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • This PR does not seem to break the templates.
  • 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?

  • Add xs.ensembles.get_partition_input to prepare data from catalog to be passed to xclim partionning functions (https://xclim.readthedocs.io/en/stable/api.html#uncertainty-partitioning)
  • Now, I am wondering if it would be more xscen to also wrap the xclim fonction instead only prepping the data? This would allow to add a attrs processing_level= partition to be able to put it in a a catalog.
    workflow 1: xs.get_partition_input -> xc.ensembles.lafferty_sriver -> fg.partition
    workflow 2: xs.partition (which gets the input and wraps xclim) -> maybe save to disk and catalog -> fg.partition or other plots (maybe maps of one component)
    EDIT: I went with workflow 1

Does this PR introduce a breaking change?

no

Other information:

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 label Dec 12, 2023
juliettelavoie added a commit to Ouranosinc/xclim that referenced this pull request Dec 14, 2023
<!--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 #1497 
- [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?

*  Add partition algo from Lafferty and Sriver (2023)

### Does this PR introduce a breaking change?
depends (see options below)

### Other information:
TODO list in this PR:

- [x] Add weights
- [x] add tests
- [x] Maybe add `num` to `hawkins_sutton` ?
- [x] Clean up based on changes outside of this PR (eg. remove functions
that are now elsewhere)

TODO list outside of this PR:

- [ ] Add a more general `graph_fraction_of_total_variance` to figanos
(in progress by Juliette,
Ouranosinc/figanos#134)
- [ ] Add a function in xscen to prepare the data from a catalog.
(Ouranosinc/xscen#289)

In this function, I could rename the dimension to fit what the partition
vocabulary (`model`, `scenario`, `downscaling`) OR we could change the
partition vocab to the catalog/xscen vocab (`source`, `experiment`,
`bias_adjust_project`). Option 2 is my personal preference, but that
would be a breaking change for `hawkins_sutton`.
@github-actions github-actions bot added the CI Modifications to autiomation utilities label Dec 15, 2023
@github-actions github-actions bot removed the CI Modifications to autiomation utilities label Dec 15, 2023
@juliettelavoie
Copy link
Contributor Author

juliettelavoie commented Dec 21, 2023

I don't understand what's going on. In commit, 2ece1d7 everything passed. I removed inconsequential lines andt the RTD build keeps failing. (I can run the notebook no problem on our servers). I have tried to rebuild the rtd a few times. In these different builds (without changing anything), the error is not always the same. Either the datasets doesn't load with a HDF5 error or it loads an empty dataset and fail when the function tries to subset ('Dataset' object has no attribute 'lon').

HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: H5A.c line 1891 in H5Aiterate2(): invalid location identifier
    major: Invalid arguments to routine
    minor: Inappropriate type
  #001: H5VLint.c line 1741 in H5VL_vol_object(): invalid identifier type to function
    major: Invalid arguments to routine
    minor: Inappropriate type
ERROR:traitlets:Kernel died while waiting for execute reply.
HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: H5A.c line 1891 in H5Aiterate2(): invalid location identifier
    major: Invalid arguments to routine
    minor: Inappropriate type
  #001: H5VLint.c line 1741 in H5VL_vol_object(): invalid identifier type to function
    major: Invalid arguments to routine
    minor: Inappropriate type
HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: H5A.c line 1891 in H5Aiterate2(): invalid location identifier
    major: Invalid arguments to routine
    minor: Inappropriate type
  #001: H5VLint.c line 1741 in H5VL_vol_object(): invalid identifier type to function
    major: Invalid arguments to routine
    minor: Inappropriate type
HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: H5D.c line 767 in H5Dget_create_plist(): invalid dataset identifier
    major: Invalid arguments to routine
    minor: Inappropriate type
HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: H5D.c line 481 in H5Dclose(): can't decrement count on dataset ID
    major: Dataset
    minor: Unable to decrement reference count
  #001: H5Iint.c line 1227 in H5I_dec_app_ref_always_close(): can't decrement ID ref count
    major: Object ID
    minor: Unable to decrement reference count
  #002: H5Iint.c line 1197 in H5I__dec_app_ref_always_close(): can't decrement ID ref count
    major: Object ID
    minor: Unable to decrement reference count
  #003: H5Iint.c line 946 in H5I_remove(): can't remove ID node
    major: Object ID
    minor: Can't delete message
  #004: H5Iint.c line 895 in H5I__remove_common(): can't remove ID node from hash table
    major: Object ID
    minor: Can't delete message
  #005: H5Iint.c line 1077 in H5I__dec_app_ref(): can't decrement ID ref count
    major: Object ID
    minor: Unable to decrement reference count
  #006: H5Iint.c line 981 in H5I__dec_ref(): can't locate ID
    major: Object ID
    minor: Unable to find ID information (already closed?)

Copy link

@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.

Pas assez familier avec xscen pour faire une révision en profondeur, mais ça me semble bon.

"metadata": {},
"source": [
"## Ensemble partition\n",
"This tutorial will show how to use the xscen to create the input for [xclim partition functions](https://xclim.readthedocs.io/en/stable/api.html#uncertainty-partitioning)."
Copy link

Choose a reason for hiding this comment

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

Suggested change
"This tutorial will show how to use the xscen to create the input for [xclim partition functions](https://xclim.readthedocs.io/en/stable/api.html#uncertainty-partitioning)."
"This tutorial will show how to use xscen to create the input for [xclim partition functions](https://xclim.readthedocs.io/en/stable/api.html#uncertainty-partitioning)."

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

LGTM! One thing that is not currently supported (but it's maybe not an issue) is regional models. If you want to delve into that, you could look at:

for k in info:

The idea is to look at driving_model instead of source, and fill that entry with source in the case of global models.

"cell_type": "markdown",
"metadata": {},
"source": [
"The function searches the catalog with `search_kw` and creates a dataset with new dimensions in `partition_dim`(`[\"source\", \"experiment\", \"bias_adjust_project\"]`). \n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need to be edited, since it still relates to the previous behaviour.

@juliettelavoie
Copy link
Contributor Author

LGTM! One thing that is not currently supported (but it's maybe not an issue) is regional models. If you want to delve into that, you could look at:

for k in info:

The idea is to look at driving_model instead of source, and fill that entry with source in the case of global models.

I don't think this is necessary for now. I will do another PR if that changes.

@juliettelavoie juliettelavoie merged commit fca2816 into main Jan 10, 2024
18 checks passed
@juliettelavoie juliettelavoie deleted the add-partition-extract branch January 10, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants