JP-3584: Use rolling window median for TSO outlier detection #8473
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8473 +/- ##
=======================================
Coverage 57.97% 57.98%
=======================================
Files 387 388 +1
Lines 38830 38927 +97
=======================================
+ Hits 22513 22572 +59
- Misses 16317 16355 +38 ☔ View full report in Codecov by Sentry. |
|
Regression tests started here. Marking this ready for review even though I still need to figure out why the readthedocs build is failing. |
|
the only regtest failure was also present here, so I don't think this PR causes that issue. However, I seem to have introduced a new unit test failure with the last small bugfix. Will track that down |
|
New regtest run started here for code changes that address comment from @braingram. The imaging mode is still refactored, but the new version should preserve the way memory is used. Let me know whether the refactor is desired - some of the refactoring is definitely extraneous to the TSO changes and could be reverted if not desired. |
Thanks for putting this together. One general question I have is: what does the refactoring of the non-tso modes provide? If it's not required for the tso changes I think it makes sense to be a separate PR where the addressed issues and improvements are described. |
|
Agree with you RE some aspects of the additional refactor, specifically the modelwise_operation decorator. I started working on it when I misunderstood exactly what was needed for the TSO modes, and then just didn't revert it when I realized the TSO modes needed something different. I do think the decorators could be a useful way to ensure that memory is taken care of responsibly when running certain oper 8000 ations, and I'd be curious to hear your thoughts on that. However it's a solution to a problem that doesn't exist yet, and may never exist. It does feel a bit strange to revert the use of I think splitting the imaging mode into its own class and inheriting from a base class makes sense to include in this PR because imaging, spec, and now TSO all use similar but distinct data processing workflows. I don't know much about the coronagraphic modes but it's possible that coronagraphy might need its own, very similar, workflow as well - this is something I have been meaning to ask @hbushouse. |
|
Thanks for the detailed response!
Thanks for the clarification. I'd be happy to discuss this.
Perhaps we could discuss this also as I'm still struggling to see how the Is the input tso data a 3D cube? If so, the entirety of the input will be read on the first call to
Splitting up the modes into sub-classes sounds great to me. I'm much less familiar with the non-image paths in this step and have been mostly focused on trying to improve the performance. I certainly do not mean to derail this PR but hopefully some of my comments have been helpful. |
|
After a long discussion with @braingram about existing memory issues with calwebb_tso3 and calwebb_image3, it's clear that this PR is attempting to do too much. It's trying to use, and make more generic, some memory optimizations that are not necessarily working correctly anyway, e.g. this issue. The proposed path forward for this JIRA ticket is to make a very straightforward change to support the rolling median calculation, without worrying about memory at all. This requires undoing a lot of the changes in this PR. Then for the future, in order to implement the above-mentioned issue in a way that will also support TSO data, I'll look into what kinds of large datasets might need to be processed through this step. |
|
Yet another regtest run started here. In this version the only refactors should be splitting some chunks of the imaging pipeline flow into their own functions to be re-used by outlier_detection_tso. I think I fixed all the other issues @braingram brought up too and I'm going to resolve those comments now. |
|
Thanks! Please do consider all of my previous comments addressed. The code changes look good to me and feel free to ping me when the regtests finish if a review would be helpful. |
|
All the regression test failures seem to be unrelated. This is ready for your review @hbushouse. You may wonder why there aren't any expected regtest failures for such a change, which should affect all data processed through calwebb_tso3. The reason is that there are no regtest datasets with >25 integrations (the default rolling window), which means that as written the code will revert to using a simple median. Another note: I did check that the unit test I wrote fails for simple median but succeeds for rolling-median. I think the fake test data are therefore similar to the real test cases where this behavior was seen. |
|
I believe the remaining regtest failures in |
|
@emolter Latest CI run had lots of failures due to the |
…ault for calwebb_tso3
Resolves JP-3584
Also resolves JP-3441
Closes #8378
Closes #8009
This PR updates the outlier detection step to use a rolling median instead of a flat median for all TSO modes, such that real time variability in the data is less likely to be flagged as outliers. The number of integrations that comprise the window for the rolling median can be specified by an input parameter to the step.
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rstwithin the relevant release sectionHow to run regression tests on a PR