spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

JP-3768: Move outlier detection median computers to stcal #292

Closed emolter closed 1 month ago

emolter commented 1 month ago

Resolves JP-3768

This PR ports the median calculation machinery added by https://github.com/spacetelescope/jwst/pull/8782 into stcal for use by other missions. See https://github.com/spacetelescope/jwst/pull/8840 for the related changes in jwst.

Tasks

news fragment change types... - ``changes/.apichange.rst``: change to public API - ``changes/.bugfix.rst``: fixes an issue - ``changes/.general.rst``: infrastructure or miscellaneous change
codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 99.23664% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.19%. Comparing base (8988d2c) to head (c2d10b7). Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/outlier_detection/median.py 98.60% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #292 +/- ## ========================================== + Coverage 84.76% 85.19% +0.43% ========================================== Files 44 46 +2 Lines 8542 8804 +262 ========================================== + Hits 7241 7501 +260 - Misses 1301 1303 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emolter commented 1 month ago

jwst regtests started here edit: no failures

emolter commented 1 month ago

@braingram I think I incorporated all your comments with the most recent push. Thanks for the docstring updates, I think they improved things a lot, and I agree with your idea to make the OnDiskMedian and DiskAppendableArray private

braingram commented 1 month ago

Would you add median.py to the docs? It will likely be easier if the submodule has an __all__ (I added a comment about this) which will also help to define the "public" API.

Was the addition of this new API to the docs not pushed?

emolter commented 1 month ago

Would you add median.py to the docs? It will likely be easier if the submodule has an __all__ (I added a comment about this) which will also help to define the "public" API.

Was the addition of this new API to the docs not pushed?

I didn't even see the comment, my fault. I will look at the docs from this branch and see what changes are needed. I did already add the __all__

braingram commented 1 month ago

Downstream romancal failures are what I'd consider unrelated. It appears the CI is using the pytest configuration from stcal when running those tests (instead of using the one in romancal). The jwst tests haven't finished but I wouldn't be surprised if it does the same thing (and is now turning some unclosed file warnings into errors).

Thanks for updating the docs, they look good to me: https://stcal--292.org.readthedocs.build/en/292/stcal/outlier_detection/index.html

I think https://github.com/spacetelescope/stcal/pull/292#discussion_r1782630616 is the only unresolved comment I made.