-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move outlier detection utility functions from jwst to stcal #270
Conversation
5afae74
to
4028dea
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
==========================================
+ Coverage 84.01% 84.32% +0.31%
==========================================
Files 39 41 +2
Lines 7345 7529 +184
==========================================
+ Hits 6171 6349 +178
- Misses 1174 1180 +6 ☔ View full report in Codecov by Sentry. |
fd15316
to
4df2447
Compare
ad63436
to
4733fb9
Compare
Closing and opening to re-trigger CI since the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few small things.
c8885c5
to
30355b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my comments have been addressed, looks good to me
c573846
to
4564878
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
looks like mypy needs an override in pyproject.toml for drizzle |
|
||
|
||
def _abs_deriv(array): | ||
tmp = np.zeros(array.shape, dtype=np.float64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added short docstrings in 2112773
This function (and _absolute_subtract
) should likely not be used since they appear to introduce erroneous cr flags due to treating off-edge pixels as 0. However, fixing this bug would result in changes to regression test results and I think makes more sense in a separate PR (where the new results can be verified). There is a JP ticket for this spacetelescope/jwst#8636 and candidate new code here https://github.com/spacetelescope/jwst/pull/8635/files
For the scope of this PR I added "Do not use this function" to the docstrings as moving this code to stcal did not introduce the bug.
|
||
|
||
def _absolute_subtract(array, tmp, out): | ||
tmp = np.abs(array - tmp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a brief docstring to this function (see #270 (comment) for more details).
I attempted to add more but given the number of lines of code and the array "switch-a-roo" that this function is performing I think reading the code is the best bet for trying to understand what's happening.
Also this function will be removed if the candidate code for fixing spacetelescope/jwst#8636 is acceptable.
Tests were unable to produce a situation where the filtered warning could be produced. The goal was to add a more specific filter (one that uses a message). If the warning can be reproduced the filter with a message can be re-added. It's possible this warning is no longer produced and so this commit removes the filter.
89cf708
to
61aeee8
Compare
This PR moves a number of utility functions related to outlier detection from jwst to stcal.
This PR is required for spacetelescope/jwst#8613
As this PR adds a few new dependencies (and these are added to the devdeps) the
run devdeps tests
label was added. It's expected that the jwst devdeps will fail as they are failing on main: https://github.com/spacetelescope/stcal/actions/runs/9836681142/job/27152917937Checklist
CHANGES.rst
(either inBug Fixes
orChanges to API
)