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

Maximum value over a height coordinate #1945

Merged
merged 9 commits into from
Oct 10, 2023

Conversation

mspelman07
Copy link
Contributor

@mspelman07 mspelman07 commented Sep 26, 2023

Related ticket: https://metoffice.atlassian.net/browse/EPPT-188
Acceptance test data: metoppv/improver_test_data#29

Adds in the ability to calculate the maximum value over a height coordinate. This includes the ability to define the bounds of the height coordinate to take the maximum over. This is so we can calculate the maximum relative humidity below 300m.

Testing:

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

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (64b8eab) 98.38% compared to head (74cb410) 98.39%.
Report is 1 commits behind head on master.

❗ Current head 74cb410 differs from pull request most recent head 087b10b. Consider uploading reports for the commit 087b10b to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1945   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files         123      123           
  Lines       11792    11804   +12     
=======================================
+ Hits        11602    11614   +12     
  Misses        190      190           
Files Coverage Δ
improver/utilities/cube_manipulation.py 99.18% <100.00%> (+0.04%) ⬆️

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

Copy link
Contributor

@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

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

A few things to think about, but nothing major.

improver/utilities/spatial.py Outdated Show resolved Hide resolved
improver/utilities/spatial.py Outdated Show resolved Hide resolved
improver/utilities/spatial.py Outdated Show resolved Hide resolved
improver/utilities/spatial.py Outdated Show resolved Hide resolved
@MoseleyS MoseleyS assigned mspelman07 and unassigned MoseleyS Sep 26, 2023
@mspelman07 mspelman07 assigned MoseleyS and unassigned mspelman07 Sep 27, 2023
Copy link
Contributor

@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

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

There is an outstanding question about meta-data, but otherwise I am happy and this PR can have a second review.

@MoseleyS MoseleyS removed their assignment Oct 3, 2023
improver/utilities/cube_manipulation.py Outdated Show resolved Hide resolved
@mspelman07 mspelman07 assigned MoseleyS and unassigned mspelman07 Oct 3, 2023
MoseleyS
MoseleyS previously approved these changes Oct 3, 2023
@MoseleyS
Copy link
Contributor

MoseleyS commented Oct 3, 2023

The acceptance test data need fixing (see their PR).

@MoseleyS MoseleyS assigned mspelman07 and unassigned MoseleyS Oct 3, 2023
MoseleyS
MoseleyS previously approved these changes Oct 5, 2023
@MoseleyS MoseleyS removed their assignment Oct 5, 2023
@brhooper brhooper self-assigned this Oct 9, 2023
Copy link
Contributor

@brhooper brhooper left a comment

Choose a reason for hiding this comment

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

Thanks @mspelman07. This mostly looks fine, I've just added one comment about adding some handling of a possible error.

height_constraint = iris.Constraint(
height=lambda height: lower_height_bound <= height <= upper_height_bound
)
cube_subsetted = cube.extract(height_constraint)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is possible for cube_subsetted to be None at this point if certain height bounds are input, which will cause an Attribute_Error on line 750.

I think handling of this issue should be added, with an error message explaining that no heights on the input cube are within the given bounds, so the max over height levels cannot be calculated. Tests should also be added for this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right this could cause a problem. I've added in an error message and a test for this.

@brhooper brhooper assigned mspelman07 and unassigned brhooper Oct 9, 2023
Copy link
Contributor

@brhooper brhooper 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 that change @mspelman07, I'm happy with this now

@brhooper brhooper merged commit 8ff0b76 into metoppv:master Oct 10, 2023
8 checks passed
@brhooper brhooper assigned mspelman07 and unassigned brhooper Oct 10, 2023
@mspelman07 mspelman07 added the EPP PR's related to Enhancing Post Processing team label Jan 9, 2024
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* Adds in plugin and tests to calculate the maximum value over a height coordinate

* formatting line length

* remove print and update docstring

* Move plugin to cube_manipulation

* formatting

* Formatting isort and flake8

* Update metadata of cube

* Update checksums for new data

* Raises an error if requested height bounds don't cover any height levels

---------

Co-authored-by: Marcus Spelman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EPP PR's related to Enhancing Post Processing team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants