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 aggregation CLI #1952

Merged
merged 32 commits into from
Oct 13, 2023
Merged

Add aggregation CLI #1952

merged 32 commits into from
Oct 13, 2023

Conversation

btrotta-bom
Copy link
Contributor

@btrotta-bom btrotta-bom commented Oct 5, 2023

Add CLI for iris aggregations sum, mean, median, std_dev, min, max. The code wraps the existing function utilities.cube_manipulation.collapse_realizations, adding the option to specify different aggregation methods (with "mean" as the default, so that existing code will continue to work).

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)
  • Added acceptance tests

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (7e2ed68) 98.39% compared to head (36fe9ed) 98.39%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1952   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         123      123           
  Lines       11808    11814    +6     
=======================================
+ Hits        11618    11624    +6     
  Misses        190      190           
Files Coverage Δ
improver/utilities/cube_manipulation.py 99.21% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benowen-bom benowen-bom self-requested a review October 9, 2023 02:16
Copy link
Contributor

@benowen-bom benowen-bom left a comment

Choose a reason for hiding this comment

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

This PR adds functionality to enable aggregation of specified dimension coordinates by suitably wrapping the collapsed function contained utilities/cube_manipulation.py.

The code has been implemented in a general fashion so as to allow aggregation of any dimension(s), as well as broadcasting back to the original dimension coordinates. However, there are some considerations around threshold coordinate based data and the metadata for broadcasted datasets that need further consideration.

Hopefully the changes to address these issues shouldn't require too much work. In the case of the threshold dimension, I'm not advocating that quantities should be evaluated via integration of the probability distribution, rather that the quantities evaluated using the current approach are not the analogues of those evaluated using "realization" or "percentile" based data.

improver/cli/aggregate_dimensions.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Show resolved Hide resolved
improver/cli/aggregate_dimensions.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Outdated Show resolved Hide resolved
improver/cli/aggregate_dimensions.py Outdated Show resolved Hide resolved
improver_tests/acceptance/test_aggregate_dimensions.py Outdated Show resolved Hide resolved
@btrotta-bom
Copy link
Contributor Author

@benowen-bom Thanks for pointing out that Improver already does broadcasting. This allows us to simplify this change considerably. The code now just slightly extends the existing collapse_realizations method to allow the user to specify the aggregation type. (We currently do not need to aggregate over any other dimensions, so I've left this functionality out.) The option to rename the cube is now handled in the cli.

improver/cli/collapse_realizations.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Outdated Show resolved Hide resolved
improver/utilities/cube_manipulation.py Show resolved Hide resolved
improver/cli/aggregate_dimensions.py Outdated Show resolved Hide resolved
Copy link
Contributor

@benowen-bom benowen-bom left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates from the previous review. The changes made have simplified the usage cases significantly and avoid the problematic cases I raised previously. I've got a few suggestions, but these are relatively minor.

Re unit-tests, given no new tests are added (at the module level), I think it's fine to use unittest rather than pytest.

benowen-bom
benowen-bom previously approved these changes Oct 10, 2023
Copy link
Contributor

@benowen-bom benowen-bom left a comment

Choose a reason for hiding this comment

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

Thanks for making those final changes. This PR looks good to me and provides a means to evaluate aggregate values from realization data, as needed for rainforests calibration.

I think rolling back the generalised elements was a good call. I think there is value in these, but they can be added when the need arises and we have the time to think through them carefully, particularly around threshold based data.

@btrotta-bom btrotta-bom added the MO review required PRs opened by non-Met Office developers that require a Met Office review label Oct 10, 2023
@gavinevans gavinevans self-assigned this Oct 12, 2023
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks @btrotta-bom 👍

I've added some minor comments.

improver/cli/collapse_realizations.py Outdated Show resolved Hide resolved
improver/cli/collapse_realizations.py Outdated Show resolved Hide resolved
improver/cli/collapse_realizations.py Outdated Show resolved Hide resolved
improver_tests/acceptance/SHA256SUMS Show resolved Hide resolved
@gavinevans gavinevans assigned btrotta-bom and unassigned gavinevans Oct 12, 2023
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @btrotta-bom 👍

@gavinevans gavinevans merged commit 2520f58 into metoppv:master Oct 13, 2023
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* add aggregation functionality

* Handle case where std_dev is undefined

* all dimensions to be a string

* update cli and acceptance tests

* Undo testing change

* formatting

* reverse testing change

* update

* change parameter name

* fix docstring

* Check dimension exists

* Refactor to use collapse-realizations cli

* Remove old file

* Handle nans

* fix bug

* Remove check

* simplify code

* Check that realization is a dimension in cli

* fix docstring

* fix docstring

* Add test for invalid method error

* Add acceptance test for when new name not specified

* formatting

* update docstring

* update docstring

* update docstring

* Use pytest

* simplify code

* formatting

* update checksum

* Remove argument.

---------

Co-authored-by: Belinda Trotta <[email protected]>
Co-authored-by: Gavin Evans <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MO review required PRs opened by non-Met Office developers that require a Met Office review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants