8000 Eppt 2354 Extend humidity mixing ratio and update standardise behaviour by katherinetomkins-mo · Pull Request #2105 · metoppv/improver · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Eppt 2354 Extend humidity mixing ratio and update standardise behaviour #2105

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 39 commits into from
Mar 19, 2025
Merged

Eppt 2354 Extend humidity mixing ratio and update standardise behaviour #2105

merged 39 commits into from
Mar 19, 2025

Conversation

katherinetomkins-mo
Copy link
Contributor
@katherinetomkins-mo katherinetomkins-mo commented Mar 10, 2025 •
edited by MoseleyS
Loading

Addresses #GitHubissuenum
https://metoffice.atlassian.net/browse/EPPT-2354

Description
Humidity Mixing Ratio extended

  • This PR extends the functionality of the humidity mixing ratio plugin to allow any cube of pressure to be used as an input to the humidity mixing ratio calculation. Previously this was restricted to a cube of "surface-air-pressure". Now any cube with "pressure" in the name will be considered as the pressure input. If there is no cube, the previous error handling is retained exactly as before. Tests and code comments have been updated to reflect this.

Virtual Temperature Units

  • As part of adding Virtual Temperature to the EPP-Workflows it was discovered that the work for the plugins that support this workflow had been done before the update to the Dagrunner dependency. As a result of testing the plugins on the latest version of EPP_Workflows, it was found that some of the previously expected behaviour had changed and the units after the Virtual Temperature calculation were no longer retained. This seems to be because of an issue with the Iris / cf-units multiplication. (See Virtual Temperature - replace Iris multiply with Numpy #2107)
    As a result it has been necessary to update the Virtual Temperature plugin to add back the expected units after the calculation use Numpy multiplication in the Virtual Temperature calculation to multiply the temperature by the humidity mixing ratio update the epp-workflows code to use the dask scheduler "processes" to avoid the race conditions encountered when using the in house "multiprocessing" scheduler.

Standardise air_temperature flag

  • f5940f4 updates the order of the metadata standardisation to allow for a situation where the air-temperature flag is not removed and the data retained. See Last minute "above_surface_pressure below_surface_pressure" coord removal #1839 for why this was implemented initially. It is now the case that we may want to use the air_temperature data, and this can be done by calling standardise and removing the status flag before continuing through the other actions. In this way, the air_temperature data is retained, and not set to NaN, to be used in calculations, here the HumidityMixingRatio.

Testing:

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

CLA

8000
  • If a new developer, signed up to CLA

@katherinetomkins-mo katherinetomkins-mo changed the title Eppt 2354 update humidity mixing ratio Eppt 2354 Ensure Virtual Temperature cube has units Mar 11, 2025
@katherinetomkins-mo katherinetomkins-mo changed the title Eppt 2354 Ensure Virtual Temperature cube has units Eppt 2354 Extend humidity mixing ratio and add units to virtual temperature Mar 11, 2025
@katherinetomkins-mo katherinetomkins-mo marked this pull request as ready for review March 11, 2025 10:33
MoseleyS
MoseleyS previously requested changes Mar 11, 2025
View reviewed changes
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.

I think a different approach to this would be more robust.

@codecov Codecov
Copy link
codecov bot commented Mar 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.45%. Comparing base (84a8944) to head (5de61e6).
Report is 80 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2105      +/-   ##
==========================================
+ Coverage   98.39%   98.45%   +0.05%     
==========================================
  Files         124      137      +13     
  Lines       12212    13631    +1419     
==========================================
+ Hits        12016    13420    +1404     
- Misses        196      211      +15

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

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mo-philrelton
mo-philrelton previously approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor
@mo-philrelton mo-philrelton left a comment

Choose a reason for hiding this comment

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

All tests pass (one exception, although this test also fails in master so I have ignored it). The code makes sense.

@katherinetomkins-mo katherinetomkins-mo changed the title Eppt 2354 Extend humidity mixing ratio and add units to virtual temperature Eppt 2354 Extend humidity mixing ratio Mar 17, 2025
@katherinetomkins-mo katherinetomkins-mo changed the title Eppt 2354 Extend humidity mixing ratio Eppt 2354 Extend humidity mixing ratio and update standardise behaviour Mar 17, 2025
mo-philrelton
mo-philrelton requested changes Mar 17, 2025
View reviewed changes
mo-philrelton
mo-philrelton previously approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor
@mo-philrelton mo-philrelton left a comment

Choose a reason for hiding this comment

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

Looks good now

mspelman07
mspelman07 requested changes Mar 18, 2025
View reviewed changes
Copy link
Contributor
@mspelman07 mspelman07 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 comments which might help simplify the code but generally I think the code is doing what we need.

mspelman07
mspelman07 requested changes Mar 19, 2025
View reviewed changes
Copy link
Contributor
@mspelman07 mspelman07 left a comment

Choose a reason for hiding this comment

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

One mis-placed comment to remove but otherwise I'm happy with the functionality of this.

@MoseleyS MoseleyS dismissed their stale review March 19, 2025 10:53

This PR has substantially changed since I reviewed it and the changes I asked for have been made.

@mspelman07 mspelman07 merged commit 37ef5fd into metoppv:master Mar 19, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mspelman07 mspelman07 mspelman07 approved these changes

@MoseleyS MoseleyS Awaiting requested review from MoseleyS

@mo-philrelton mo-philrelton Awaiting requested review from mo-philrelton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@katherinetomkins-mo @MoseleyS @mspelman07 @mo-philrelton
0