search

ioos / ioos_qc

:ballot_box_with_check: :ocean: IOOS QARTOD and other Quality Control tests implemented in Python
https://ioos.github.io/ioos_qc/
Apache License 2.0
46 stars 27 forks source link

Initial commit of the density inversion test #42

Closed JessyBarrette closed 3 years ago

JessyBarrette commented 3 years ago

We present here an additional test within the QARTOD module which corresponds to the QARTOD Test 13 Density Inversion Test. image

This implementation can be applied to both up, down, and up&down cast situations, which can be handy in CTD profiling and trajectory profiling applications like Rosette deployment, stand-alone CTD instruments profile, glider, AUV, automated profilers (ex: Argo program), and towed instrumentation.

The test is very similar to the ARGO density inversion test and respects the QARTOD codable instructions.

QARTOD documentation suggests that flag results can only be flagged as GOOD(1) or BAD(4), ignoring the SUSPECT (3). We also provide more flexibility to the user by also providing a non-mandatory SUSPECT threshold value.

The same test could live potentially within eh ARGO module. I'll leave to the ioos_qc community the decision if it makes sense to leave it within the QARTOD module, or move it to the ARGO module.

kwilcox commented 3 years ago

Thanks @JessyBarrette, the CI failures were in master (fixed now) and not in this PR.

Could you please add a test that uses the density inversion tests? See the other QARTOD tests for examples. After that this looks great!

JessyBarrette commented 3 years ago

Awesome! Yes I can certainly do that!

JessyBarrette commented 3 years ago

I think those tests should cover most the potential issues that could be encountered. I aslo found some issues with the density_inversion_test itself which were corrected. Let me, know if you find any issues with it!

kwilcox commented 3 years ago

@JessyBarrette The test_density_inversion_bad_depth_value test is failing in CI and for me locally. The check is catching warnings when converting the depth so I don't think this warning will ever happen.

We could move the zinp masking outside of the with warnings... block or remove the warning check from the test... up to you!

kwilcox commented 3 years ago

I dug a little deeper, I think the Warning you were trying to catch was unrelated to the None value in depths. It was coming from the usage of .astype(np.floating) which is producing:

DeprecationWarning: Converting `np.inexact` or `np.floating` to a dtype is deprecated. The current result is `float64` which is not strictly correct. return array(a, dtype, copy=False, order=order)

The usage of .astype(np.floating) is prolific in the codebase and I will change to np.float64 in another PR!

JessyBarrette commented 3 years ago

Thanks Kyle for your corrections! I was thinking of adding a warning in the density_inversion test itself when zinp is missing a value and forgot to add the warning in qartod.density_inversion_test. I'm not sure if that is necessary since there isn't a similar feature in the other tests if like zinp, lat, or long have a None value.

Looks like you dropped that warning that's great, I guess it follows more what's been done with the other tests. I'm happy with that