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

MOBT-661: Required changes for precipitation phase accumulations #1997

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

brhooper
Copy link
Contributor

@brhooper brhooper commented May 15, 2024

Related to: https://github.com/metoppv/mo-blue-team/issues/661, https://github.com/metoppv/mo-blue-team/issues/671
Acceptance test data: metoppv/improver_test_data#48

Description
This PR includes a few changes which are required for the work to produce precipitation phase accumulations over longer periods.

Included changes are as follows:

  • Add option for 'realization' tie-breaking within ECC. This resolves ties within the dependence template by assigning values to the highest realization number.
  • Factor out code which enables recycling realizations in the ECC dependence template.
  • Create a CLI for the above recycling code to enable extending the realization dimension on phase probability cubes. This ensures realization recycling is done using a consistent method, which is important for precipitation phase accumulation diagnostics for reasons laid out in this comment (https://github.com/MetOffice/improver_aux/pull/420#issuecomment-2051865950).
  • Make one of the checks in the combine plugin easier to pass - test will no longer fail if coordinates on input cubes differ in the var_name, except if the difference is on a threshold coordinate. This mirrors, and was necessitated by, the functionality of the MergeCubes plugin, which removes this attribute from coordinates.
  • Add functionality to 'recursive_filter' plugin to allow application to cubes containing different masks on each spatial slice. Also adds this functionality to the corresponding CLI.
  • Add unit and acceptance tests where required for each of the above.

Testing:

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

@brhooper brhooper changed the title Required changes for precipitation phase accumulations MOBT-661: Required changes for precipitation phase accumulations May 15, 2024
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.37%. Comparing base (84a8944) to head (f7aff40).
Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1997      +/-   ##
==========================================
- Coverage   98.39%   98.37%   -0.03%     
==========================================
  Files         124      133       +9     
  Lines       12212    12903     +691     
==========================================
+ Hits        12016    12693     +677     
- Misses        196      210      +14     

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

@bayliffe bayliffe self-assigned this May 16, 2024
@brhooper brhooper assigned brhooper and unassigned bayliffe May 31, 2024
Copy link

In order to maintain a backlog of relevant PRs, we automatically label them as stale after 60 days of inactivity.

If this PR is still important to you, then please comment on this PR and the stale label will be removed.

Otherwise this PR will be automatically closed in 30 days time.

@github-actions github-actions bot added the Stale label Sep 21, 2024
@brhooper
Copy link
Contributor Author

Commenting to remove 'staleness'.

@brhooper brhooper force-pushed the mobt_671_ecc_extensions branch 2 times, most recently from 873974b to 46ccc84 Compare October 3, 2024 13:38
@brhooper brhooper removed their assignment Oct 3, 2024
@gavinevans gavinevans self-assigned this Oct 3, 2024
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 @brhooper 👍

I've added some comments and suggestions.

improver/nbhood/recursive_filter.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_tests/acceptance/test_generate_realizations.py Outdated Show resolved Hide resolved
improver/cli/manipulate_realization_dimension.py Outdated Show resolved Hide resolved
improver_tests/acceptance/SHA256SUMS Outdated Show resolved Hide resolved
improver_tests/acceptance/SHA256SUMS Show resolved Hide resolved
@gavinevans gavinevans assigned brhooper and unassigned gavinevans Oct 4, 2024
@brhooper brhooper assigned gavinevans and unassigned brhooper Oct 10, 2024
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 @brhooper 👍

I've happy to approve these changes, subject to the GitHub Actions failures being addressed.

@gavinevans gavinevans assigned brhooper and unassigned gavinevans and brhooper Oct 10, 2024
…Add acceptance test and data for this functionality. Update checksums. Change numpy broadcasting method in plugin.
…dimension coordinate var_name attributes does not cause an error, except where the var_name is different on a threshold coordinate. Change made to better work with the functionality of MergeCubes, which removes var_name from non-threshold dimension coordinates.
@brhooper
Copy link
Contributor Author

Thanks @brhooper 👍

I've happy to approve these changes, subject to the GitHub Actions failures being addressed.

I've pulled the changes from this PR into my PR. The GIthub actions now succeed as expected following this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants