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

attenuated signal: add `min_period` option (alternative to `min_obs`) #29

Closed jessicaaustin closed 4 years ago

jessicaaustin commented 4 years ago

Exploring the idea of using min_period (minimum number of seconds in window required to calculate a result) instead of min_obs (minimum number of observations in window required to calculate a result).

I duplicated the notebook example and checked in the output, so you can view them alongside. This PR is to illustrate this idea, and should NOT be merged in as-is.

review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

jessicaaustin commented 4 years ago

I rebased to latest master. Looks like these changes decreased performance.

Master: https://travis-ci.com/github/ioos/ioos_qc/jobs/290526522#L939 Branch: https://travis-ci.com/github/ioos/ioos_qc/jobs/298628352#L917

So I need to look into that.

kwilcox commented 4 years ago

"I really enjoy debugging the Travis CI process" - nobody ever

kwilcox commented 4 years ago

@jessicaaustin performance improved, good to merge?

jessicaaustin commented 4 years ago

Since min_obs was only introduced recently and has never been released, nobody is using it. So if you and @yosoyjay are ok with min_period instead, I'd rather remove min_obs altogether. This PR is just a proof of concept right now to demonstrate the difference.

If you guys want to keep min_obs then I'll update the docs to make it clear you can choose one or the other.

(also, this PR has notebook outpots checked in; i need to remove those too before merging)

yosoyjay commented 4 years ago

Generally, I could see the need for min_obs. But if the goal is to deprecate that, this implementation is probably a reasonable approach.

jessicaaustin commented 4 years ago

Cleaned up; will merge once CI passes