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

port change from #35 #104

Closed
wants to merge 2 commits into from
Closed

port change from #35 #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
435553c
port change from #35
ocefpaf Mar 15, 2024
f2ec6b2
my guess is that this should be 3, fail
ocefpaf Mar 15, 2024
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
5 changes: 3 additions & 2 deletions ioos_qc/qartod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,9 @@ def check(self, tinp, inp, zinp):
with np.errstate(invalid='ignore'):
z_idx = (~zinp.mask) & (zinp >= m.zspan.minv) & (zinp <= m.zspan.maxv)
else:
# Only test the values with masked Z, ie values with no Z
z_idx = zinp.mask
# If there is no z data in the config, don't try to filter by depth!
# Set z_idx to all True to prevent filtering
z_idx = np.ones(inp.size, dtype=bool)

# Combine the T and Z indexes
values_idx = (t_idx & z_idx)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_qartod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ def test_climatology_test_depths(self):
101
)
]
expected_result = [1, 1, 1, 3, 3, 2]
expected_result = [1, 1, 1, 3, 3, 3]
Copy link
Member Author

Choose a reason for hiding this comment

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

My guess is that this one should "fail" (marked as 3, right?) b/c the z-values are outside of the range. @eldobbins I'm not sure you are still working on this but it seems that you were the one looking into this problem in #65.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ocefpaf, I would think we want that last result to be a 2 for "QARTOD not applied" since the data does not include information about depths so we can not apply QARTOD, instead of 3 for "QARTOD applied and this data point is a bad data point and failed the test".

My interpretation of QARTOD manuals is that 3 should be reserved for data that has been identified as bad. But in the context of the climatology test, this is more just that we are unable to use the climatology test on this dataset.

(Note: Liz is no longer with Axiom)

Copy link
Contributor

@iwensu0313 iwensu0313 Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://cdn.ioos.noaa.gov/media/2020/07/QARTOD-Data-Flags-Manual_version1.2final.pdf
Screenshot 2024-03-19 at 1 09 44 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I guess we need to look again into #35. I'll mark this one as a draft for now. Feel free to edit/resend it if you see a clear path forward.

Copy link
Contributor

@iwensu0313 iwensu0313 Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, okay -

TLDR: So a flag of 3 does make sense (good w/ the change you made), bc we did do a check on the value 79 against the only applicable vspan ((40,50)). A configured vspan is applicable if data point falls in the tspan (time range) and zspan (depth range, where provided). If zspan is not provided, it will ignore the depth value and only match against the tspan.

Details (and so I don't have to re-remember in the future; feel free to ignore):
I spent some more time in the ioos_qc.qartod and the test_qartod unit tests this morning in debug mode to better understand how the test configurations are working (prior to Axiom, my interaction with ioos_qc was more of a user, applying it to ocean data and had never looked into the codebase before to see how it actually works behind the curtains).

(Looking at the screenshot above that I posted, 3 is actually Suspect not Fail! Not sure how I missed that last week)

Regarding the last test data point:

            # Not run, has depth that's outside of given depth ranges
            (
                np.datetime64('2012-01-02'),
                79,
                101
            )

The only climatology config that gets applied is this one (line 735):

        self.cc.add(
            tspan=(np.datetime64('2012-01'), np.datetime64('2013-01')),
            vspan=(40, 50),
        )

In this unit test, the code basically first checks to see if the test data falls within the time and depth range provided (of each of the 5 configs in setUp lines 725-750) to determine if the config should be applied to this test data. If it falls within specified time and depth ranges, then it will run the test for the value (in this case 79) against the vspan ((40,50)) to see if it is within/outside the range. And since we don't set fspan in any of the 5 configs in this unit test, if a data point falls outside of vspan, it will get a 3. It can only get a 4 if we set fspan.

If the depth associated with data point we are testing (e.g. 101) is outside of any given depth ranges (e.g. (0,10), (10,100)), the data point (e.g. 79) will still be tested if there is a config w/ no zspan specified AND the time associated with the data point (e.g. np.datetime64('2012-01-02')) falls within the tspan. The climatology test does not and should not flag a data point as suspect or fail if the depth falls outside of the given depth range. Only if the value falls outside of the relevant vspan. No zspan specified means it will be applied to a data point regardless of the depth IF the time associated with that data point falls within tspan.

self._run_test(test_inputs, expected_result)


Expand Down
Loading