-
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
Add collapse operator with corresponding yaml file and changes #168
Conversation
renaming the aggregate operator to collapse operator as it is doing a collaps using min, max, percentile functions. |
At a quick glance the operator looks reasonable. I'll do a more detailed review later today. |
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.
Generally looks good to me. Some minor changes suggested around the code. I did also wonder whether we wanted to generalise the operator around how many dimensions it can collapse at once. See my comment on the collapse_cube_2dim function.
To get the tests passing this branch needs rebasing on main. You can do that as follows: git pull
git rebase origin/main aggregator_operator
git push --force |
3779485
to
a6e77ef
Compare
Hi, I have gone through your suggestions and pushed the changes. Could you take a look please? I had already started on the aggregate operator so hope none of the aggregator related code has made it into this push. |
a6e77ef
to
3779485
Compare
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>
3779485
to
ac7fe99
Compare
I've reformatted the code, removed the unnecessary collapse_2dim operator, and gotten the tests passing. The 1 dim function already actually handled collapsing over multiple coordinates, so I just updated its docstring. |
Only thing left to do is to add a test for the PERCENTILE method, and it should be ready to merge. |
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.
I'm happy with this now. @Sylviabohnenstengel if you are happy, go ahead and merge in this PR.
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.
Do we still use the test_data/*yaml files?
Review requested for when you are available next week.
first draft of aggregator dealing with min, max, stdev, etc.
The percentile aggregator is included with further line in yaml file and if condition in aggregator using the **kwargs argument. Interested in solution that looses the if condition when you review the code.
Fixes #99