port change from #35 - Githubissues

ocefpaf commented 4 months ago

This PR ports the only missing change from #35. The problem is that I'm not familiar with the expected results to fix the failing tests. With this change this test case passes but I cannot confirm the original poster test case.

iwensu0313 commented 3 months ago

I just approved. But double checking that the changes in this PR work with the example cases provided here https://github.com/ioos/ioos_qc/issues/65

iwensu0313 commented 3 months ago

I just approved. But double checking that the changes in this PR work with the example cases provided here #65

Okay, we have to look into one more thing (but I need to pause and come back to this). I tested the test code snippet provided by lgarzio in #65 with the changes in this PR and it's still not producing the expected result (it does work when you include z_span). code snippet:

from ioos_qc.qartod import ClimatologyConfig, climatology_test
c = ClimatologyConfig()
c.add(
  tspan=['2021-06-01', '2021-09-01'], 
  vspan=[3.4, 5]
  )

inp = np.array([0.     ,     np.nan, 0.     ,     np.nan,     np.nan,     np.nan,     4.16743, 4.23101,     4.23322])

t = np.array(['2021-07-16T19:01:01.313999872', '2021-07-16T19:01:02.315000064',
       '2021-07-16T19:01:02.903000064', '2021-07-16T19:01:03.903000064',
       '2021-07-16T19:01:04.903000064', '2021-07-16T19:01:05.903000064',
       '2021-07-16T19:01:06.903000064', '2021-07-16T19:01:07.336000000',
       '2021-07-16T19:01:07.903000064'], dtype='datetime64[ns]')

z = np.array([0.     ,     np.nan, 0.     ,     np.nan,     np.nan,     np.nan,     0.08931513, 0.15878244,     0.11908684])

result = climatology_test(config=c,
                        inp=inp,
                        tinp=t,
                        zinp=z)

we should get this result (bc 4 data points are np.nan):

masked_array(data=[3, 9, 3, 9, 9, 9, 1, 1, 1],
             mask=False,
       fill_value=999999,
            dtype=uint8)

but I get

masked_array(data=[3, 1, 3, 1, 1, 1, 1, 1, 1],
             mask=False,
       fill_value=999999,
            dtype=uint8)

iwensu0313 commented 3 months ago

Alright, I have a couple suggested changes:

Adding a test to capture the missing data scenario described above, first mentioned in #65 (it would pass the missing data scenario to self._run_test and ensure that the flag is 9 not 1)
Moving flag_arr[inp.mask] = QartodFlags.MISSING from the top to the bottom of check() in qartod.py, which will fix it, but there's one more behavior that is kinda funky (can explain in a follow on comment)

I tried to push my local changes for the above two things to your branch here, but I got this message bc I don't have write permissions for ioos_qc

How should I proceed?

ocefpaf commented 3 months ago

I tried to push my local changes for the above two things to your branch here, but I got this message bc I don't have write permissions for ioos_qc

I don't have enough permissions to add you but I'll request folks who do to add you. Meanwhile you can:

Send edits to my fork's branch (IMO that is too complicated if you don't want to mess with git);
Just send a fresh PR copying my changes. I don't care about authorship of the commits but I do care about getting you (and others) up to speed with ioos_qc.

IMO 2 is easier. Let me know what you want to do.

ocefpaf commented 3 months ago

Closing this one in favor of #107.

ioos / ioos_qc

port change from #35 #104