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

IM-Suite-2102: Threshold bug fix for vicinity processing of masked fields #2071

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

Conversation

bayliffe
Copy link
Contributor

@bayliffe bayliffe commented Dec 19, 2024

Bug fix to threshold plugin.
When applying fuzzy thresholds we use a rescaling that uses numpy.where. This does not exclude masked points. The fill values beneath the mask are typically 1E36. These points always exceed any threshold and are rescaled to probabilities of 1.0 for all thresholds.

If a masked field is processed with fuzzy thresholds, and uses a vicinity radius, then these probabilities of 1 given by the masked area are pulled into the unmasked area around the edge of the masking up to the vicinity distance. This results in a permanent ring of probabilities of 1 around the edge of a masked region at all thresholds.

This bug originates in the redesign of the threshold method last year in which the re-application of the mask to the truth values prior to the vicinity application but after rescaling was lost.

I've added a new acceptance test to cover this: test data: metoppv/improver_test_data#68

Testing:

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

…ty processing following last year's refactor. This corrects the issue.
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.40%. Comparing base (84a8944) to head (2d0dac0).
Report is 54 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2071    +/-   ##
========================================
  Coverage   98.39%   98.40%            
========================================
  Files         124      134    +10     
  Lines       12212    13146   +934     
========================================
+ Hits        12016    12936   +920     
- Misses        196      210    +14     

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

@bayliffe bayliffe marked this pull request as ready for review December 19, 2024 15:22
@gavinevans gavinevans self-requested a review December 20, 2024 12:43
@gavinevans gavinevans self-assigned this Dec 20, 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 @bayliffe 👍

I've added one very minor comment, but otherwise this looks fine.

Comment on lines +226 to +229
acc.kgo_root()
/ "threshold"
/ "nowcast"
/ "precip_accumulation_thresholds.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

You could specify this above the args call, particularly given that kgo_dir already exists as a variable e.g.
config_path = kgo_dir / "precip_accumulation_thresholds.json".

@gavinevans gavinevans assigned bayliffe and unassigned gavinevans Dec 20, 2024
@Kat-90 Kat-90 self-assigned this Jan 3, 2025
Copy link
Contributor

@Kat-90 Kat-90 left a comment

Choose a reason for hiding this comment

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

Separate to Gavin's comment. Reviewed the code and ran the tests alongside the test_data changes. All looks good to me.

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