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

Climatological op #290

Merged
merged 73 commits into from
Dec 19, 2023
Merged

Climatological op #290

merged 73 commits into from
Dec 19, 2023

Conversation

vindelico
Copy link
Collaborator

@vindelico vindelico 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?

xscen.aggregate.climatological_mean is replaced by xscen.aggregate.climatological_op.

climatological_op permits to apply operations ('op') other than 'mean' to the input dataset.
operations implemented: ['max', 'mean', 'median', 'min', 'std', 'sum', 'var', 'linregress']

other additions:

  • argument 'min_periods' can be passed as a 0 < real value <= 1 to restrict calculation to a percentage of available values in a period.
  • argument 'interval' has been renamed to 'stride'
  • flag 'rename_variables' == True will rename variables in output to {input_var_name}_clim_{op} to facilitate combining output from multiple operations in one ds.
  • flag 'periods_as_dim' == True will restructure the output with periods and {freq} as new coordinates and dimensions.

Modifiede the "Getting Started" notebook to use climatological_op with option 'mean'.

Does this PR introduce a breaking change?

No, climatological_mean is retained and calls climatological_op.
Tests for climatological_mean were adjusted for minor changes in attribute values.

Other information:

Uses a wrapper for scipy.stats.linregress for trend calculation with xarray.

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 Nov 19, 2023
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.

it will be great to have all those options ! thanks!

xscen/aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
tests/test_aggregate.py Outdated Show resolved Hide resolved
tests/test_aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Show resolved Hide resolved
xscen/indicators.py Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
vindelico and others added 4 commits December 5, 2023 15:34
### 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).
- [x] (If applicable) Tests have been added.
- [x] This PR does not seem to break the templates.
- [x] CHANGES.rst has been updated (with summary of main changes).
  - [x] Link to pull request (:pull:`number`) has been added.

### What kind of change does this PR introduce?

Adds the function `xscen.indicators.select_inds_for_avail_vars` to
filter the indicators that can be calculated with the variables
available in a `xarray.Dataset`.

### Does this PR introduce a breaking change?

No.

### Other information:
CHANGES.rst Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
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! This will be a great addition.

"- The `cat:frequency` and `cat:xrfreq` attributes remain unchanged.\n",
"\n",
"\n",
"Alternatively, setting the `horizons_as_dim` argument to *True* will rearrange the dataset with a new dimension `period` and a dimension named according to the temporal aggregation (`year`, `season` or `month`). The time stamps are conserved in the `time` coordinate as an array with those two new dimensions."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Alternatively, setting the `horizons_as_dim` argument to *True* will rearrange the dataset with a new dimension `period` and a dimension named according to the temporal aggregation (`year`, `season` or `month`). The time stamps are conserved in the `time` coordinate as an array with those two new dimensions."
"Alternatively, setting the `horizons_as_dim` argument to *True* will rearrange the dataset with a new dimension `horizon` and a dimension named according to the temporal aggregation (`year`, `season` or `month`). The time stamps are conserved in the `time` coordinate as an array with those two new dimensions."

Copy link
Collaborator

Choose a reason for hiding this comment

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

and a dimension named according to the temporal aggregation (year, seasonormonth)

If year has been removed, it would need to be removed here too.

else:
op_kwargs = {
"keep_attrs": True
} # ToDo: Is there a global setting in xscen so we don't need this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that calling with xr.set_options(keep_attrs=True): would work in a workflow.

This is something we discussed a while ago, and having a blanket statement for keep_attrs=True would probably not be advised. It's better to do it as needed, such as here.

Suggested change
} # ToDo: Is there a global setting in xscen so we don't need this?
}

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.

great!

@vindelico vindelico merged commit 9bb7d56 into main Dec 19, 2023
18 checks passed
@vindelico vindelico deleted the climatological_op branch December 19, 2023 20:06
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