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

Aggregation functions and other minor fixes #35

Merged
merged 62 commits into from Aug 22, 2022
Merged

Conversation

RondeauG
Copy link
Contributor

@RondeauG RondeauG commented Jul 26, 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?

Main changes

  • Major API changes from another branch.
  • New file: aggregate.py
    • aggregate.climatological_mean computes rolling means over a specified window.
    • aggregate.deltas computes deltas.
    • aggregate.spatial_mean computes a spatial means using either .mean(), .interp() or xesmf.SpatialAverager()
  • compute_indicators now supports disjointed time periods.

Minor changes

Does this PR introduce a breaking change?

  • Both shapely and geopandas would be requirements for xscen. Are we fine with that?

Other information:

@Zeitsperre Zeitsperre added the documentation Improvements or additions to documentation label Jul 28, 2022
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.

Cool!
Are you planning on adding a section to GettingStarted ?
I will try a few tests of your functions on my data.

xscen/__init__.py Show resolved Hide resolved
xscen/indicators.py Outdated Show resolved Hide resolved
xscen/indicators.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 Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
@RondeauG
Copy link
Contributor Author

RondeauG commented Aug 1, 2022

Cool! Are you planning on adding a section to GettingStarted ? I will try a few tests of your functions on my data.

I was thinking of making a specific Notebook with aggregate and reduce, focusing on their use for ensemble selection. I was waiting until everything is ready, but I could start working on it if you want to.

@juliettelavoie
Copy link
Contributor

I agree that reduce should be in its own notebook. But, I feel like climatological mean and deltas have their place in Getting Started between indicators and ensemble.

@juliettelavoie
Copy link
Contributor

For compute_deltas, the time axis is missing the horizon of reference. Is this on purpose ? It will always be 0 (or 1), but I would be encline to keep it.

  1. It is better to have regular axis without holes.
  2. We need it for ccdp.
  3. It is a good validation of what the data is.

@RondeauG
Copy link
Contributor Author

RondeauG commented Aug 1, 2022

For compute_deltas, the time axis is missing the horizon of reference. Is this on purpose ? It will always be 0 (or 1), but I would be encline to keep it.

  1. It is better to have regular axis without holes.
  2. We need it for ccdp.
  3. It is a good validation of what the data is.

It was on purpose since I didn't see the point of keeping it, but I can change that since you have a use case.

@RondeauG
Copy link
Contributor Author

In the code that I copied for compute_deltas, the variables are always renamed. For example, growing_degree_days becomes growing_degree_days_delta_1981_2010. What do we think of that? Should we make it an option?

@juliettelavoie
Copy link
Contributor

In the code that I copied for compute_deltas, the variables are always renamed. For example, growing_degree_days becomes growing_degree_days_delta_1981_2010. What do we think of that? Should we make it an option?

Making it optional is okay with me. The variable was always renamed because it was always in the same dataset has the 30y mean (no delta). But, in xscen, the mean and the deltas are separate.

1 similar comment
@juliettelavoie
Copy link
Contributor

In the code that I copied for compute_deltas, the variables are always renamed. For example, growing_degree_days becomes growing_degree_days_delta_1981_2010. What do we think of that? Should we make it an option?

Making it optional is okay with me. The variable was always renamed because it was always in the same dataset has the 30y mean (no delta). But, in xscen, the mean and the deltas are separate.

@RondeauG
Copy link
Contributor Author

This PR should be OK for a new round of reviews. I also updated the notebooks.

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.

Je n'ai pas testé les fonctions, mais ça me semble beau!

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.

looks good!

xscen/aggregate.py Outdated Show resolved Hide resolved
xscen/aggregate.py Outdated Show resolved Hide resolved
@RondeauG RondeauG merged commit 4b20db1 into main Aug 22, 2022
@RondeauG RondeauG deleted the ensemble-selection branch August 22, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants