-
Notifications
You must be signed in to change notification settings - Fork 1
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
Including aggregate operator. #241
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach is good, but we need to decide if we are just aggregating over time, or an arbitrary coordinate, and once that is decided changes need to be made to fit that decision.
Whitespace cleanup and removal of debugging print statements.
64f9c99
to
41d71db
Compare
I've pushed an update to the aggregate function that makes it more general, though I'm not entirely sure this is the way we want to go, as it may instead be better to have specific functions for aggregating over time, etc, as we would then be able to make them nicer to use. |
Based on discussion with are going with a single aggregation file that contains separate functions for time and horizontal and vertical aggregation as we then need to consider weighting functions as well. |
included timedelta functionality. This requires pip install isodate and I included that in pyproject and environments.yaml. |
Because the dependencies changed the conda lock files need updating. Will do that now. |
[CI] Update conda lock files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nearly there. Lots of small style comments, but the main change is just renaming the operator from aggregate
to time_aggregate
now it is more specific.
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
@jfrost-mo thanks for pointing out the rest :-) all changes made as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with suggestions
Just want to get the tests passing now, ahead of merging. I think I just caught the last aggregate rather than time_aggregate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with changes
still to be implemented: |
… further development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully all changes addressed
Just need to get the tests passing now. But it seems there is a difference in the number of coordinates... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are passing, and code looks good. Go ahead and merge! 🚀
It requires a modification to the plot operator to allow spatial plots from 3D cubes that contain timeseries data.
#240
Fixes #169