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

Bias correction fix #1969

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

benowen-bom
Copy link
Contributor

@benowen-bom benowen-bom commented Nov 22, 2023

Addresses #1968.

This PR updates the way masks are inherited from the forecast and truth cubes to the forecast_error. Rather than happening implicitly (as is currently the case), the propagation of masks is now performed explicitly.

Unit and acceptance tests were also updated to considered masked data (for both forecast and truth) to ensure the masks are passed on as expected.

The updated acceptance test data is included in metoppv/improver_test_data#36.

Testing:

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

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3eee0a9) 98.40% compared to head (1b5c73c) 98.40%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1969   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         124      124           
  Lines       12028    12030    +2     
=======================================
+ Hits        11836    11838    +2     
  Misses        192      192           

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

Copy link
Contributor

@dmentipl dmentipl left a comment

Choose a reason for hiding this comment

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

Thanks @benowen-bom this addresses #1968. The addition of tests checking for masked arrays vs non-masked arrays is good. I have checked that the acceptance tests pass locally. I understand that @bomRob has run a suite on a workstation to check that the results are as expected.

@dmentipl dmentipl merged commit b8e8d46 into metoppv:master Nov 27, 2023
10 checks passed
@benowen-bom
Copy link
Contributor Author

Thanks @benowen-bom this addresses #1968. The addition of tests checking for masked arrays vs non-masked arrays is good. I have checked that the acceptance tests pass locally. I understand that @bomRob has run a suite on a workstation to check that the results are as expected.

Thanks @dmentipl for the review!

MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* Combine masks explicitly when calculating forecast error.

* Update acc-tests to capture masked domains in calculate_forecast_bias

* Simplify mask handling.
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.

2 participants