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.

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

Fix flagging logic for missing and unknown data in QARTOD spike test #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix flagging logic for missing and unknown data in QARTOD spike test #129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions ioos_qc/qartod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,9 +611,17 @@ def spike_test(
flag_arr[0] = QartodFlags.UNKNOWN
flag_arr[-1] = QartodFlags.UNKNOWN

# If the value is masked or nan set the flag to MISSING
flag_arr[diff.mask] = QartodFlags.MISSING

# Check if the original data was masked
for i in range(inp.size):

# Check if both inp and diff are masked
if inp.mask[i] and diff.mask[i]:
flag_arr[i] = QartodFlags.MISSING

# Check if either inp or diff is masked
elif inp.mask[i] or diff.mask[i]:
flag_arr[i] = QartodFlags.UNKNOWN
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi there, I was looped into the discussion in issue 106, and @ocefpaf recommended I take a look at this PR if I had any thoughts.

This looks good!

Even though the resulting flag_arr is the same, I would think we'd want the conditionals to look like this, no?

       # Check if inp is masked (original data missing)
        if inp.mask[i]:
            flag_arr[i] = QartodFlags.MISSING
        
       # Check if diff is masked but not in inp (this indicates that original data is not missing, 
       # but the data point got masked in diff lines 575-580 due to trying to calculate a value 
       # using a valid value and a missing value; and because of that, we are not able to apply QARTOD
       # thus the UNKNOWN flag)
        elif (diff.mask[i] and not inp.mask[i]):
            flag_arr[i] = QartodFlags.UNKNOWN

@Sakshamgupta90

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the corrected behavior to what is expected, we will also want to change the expected list in test_qartod.QartodSpikeTest.test_spike_masked() (line ~1021) to this:

expected = [2, 4, 4, 4, 1, 3, 1, 2, 9, 2, 4, 4, 2, 9]

Previously, it was applying the MISSING FLAG 9 to the values surrounding actual missing values. Your suggested change fixes this and we should see UNKNOWN FLAG 2 surrounding missing values instead

Copy link
Contributor

@iwensu0313 iwensu0313 Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(similar updates need to be made to the expected outputs in test_spike_methods() and test_spike_test_inputs(). after which I believe all the tests should pass - I was able to pass them locally w/ the minor updates)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @iwensu0313 ,
I completely agree with you. The conditionals should indeed be structured as you've suggested to accurately reflect the intended behavior.
Additionally, updating the expected results list is necessary to ensure the test remains valid.

Thank you for pointing out the required changes.


return flag_arr.reshape(original_shape)


Expand Down
Loading