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

Update decision tree to include an option for handling masked points #2035

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

Conversation

mspelman07
Copy link
Contributor

@mspelman07 mspelman07 commented Sep 30, 2024

Addresses: https://metoffice.atlassian.net/browse/EPPT-1443

This PR updates the categorical decision tree plugin to include the option to add an "if_masked" key to a node. This defines what node masked points should be taken to.

This should not change the default behaviour of the decision tree so there's no need for the IMPROVER weather symbols decision tree to be updated.

Testing:

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

@mspelman07 mspelman07 added the EPP PR's related to Enhancing Post Processing team label Sep 30, 2024
@mspelman07 mspelman07 self-assigned this Sep 30, 2024
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.34%. Comparing base (84a8944) to head (8b34882).
Report is 28 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2035      +/-   ##
==========================================
- Coverage   98.39%   98.34%   -0.05%     
==========================================
  Files         124      132       +8     
  Lines       12212    12842     +630     
==========================================
+ Hits        12016    12630     +614     
- Misses        196      212      +16     

☔ 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.

I think the actual code change is fine, but the unit test isn't doing what you think it is doing and I'm pretty sure it is passing for the wrong reasons.


expected_mask = np.full_like(precip_cube.data, False)
expected_mask[0, 0] = True
expected = np.ma.masked_array(expected_array, mask=expected_mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is suspicious. The test at the end looks at expected_array and yet this test still passes. This problem is hidden by another problem in the test at the end - see later comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the decision tree slightly so the if masked will go to false rather than true which would stop this test passing.

@@ -74,9 +74,12 @@ accessed with this key contains the essentials that make the node function.
must be exceeded or not exceeded (see threshold_condition) for the node to
progress to the succeed target. Two values required if condition_combination
is being used.
- **threshold_condition** (str): Defines the inequality test to be applied to
- **threshold_condition** (str or list(str)): Defines the inequality test to be applied to
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 we should have a unit test that demonstrates this new behaviour explicitly, rather than via the if_masked functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've separated the masked data tests into their own test rather than parameterising it which I think is what you were asking for?

improver/categorical/decision_tree.py Show resolved Hide resolved
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.

2 participants