JP-4174: Fix point_inside_ellipse returns True for points outside the ellipse #456
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
- Coverage 89.89% 89.88% -0.01%
==========================================
Files 65 65
Lines 9983 9977 -6
==========================================
- Hits 8974 8968 -6
Misses 1009 1009 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
RT runs are linked above
|
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
As you can see, I also did some code clean-ups so it is easier on my eyes when I worked on the modules. For larger scale clean-up discussions, see spacetelescope/jwst#10081 .
I also removed some outdated "cv" or "cv2" wordings that should have been done as part of #138
I also added Brett Graham as co-authored on the commit that adds his test cases.
src/stcal/jump/jump.py
Outdated
|
|
||
| import numpy as np | ||
| import astropy.stats as stats | ||
| import numpy as np |
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
Unrelated to the bug but doesn't hurt? xref spacetelescope/jwst#9646
|
|
||
| # also apply to the after_jump thresholds | ||
| # XXX Maybe move this computation | ||
| # Ken M: Maybe move this computation |
There was a problem hiding this comment.
"XXX" means nothing to me and I think it is Ken's signature comment, so changed to his name. If I am mistaken, please let me know.
| assert np.all(gdq[0, 4, 12:22, 14:23]) == 0 | ||
|
|
||
|
|
||
| def test_inside_ellipse5(): |
There was a problem hiding this comment.
Seeing def test_inside_ellipse5() and def test_inside_ellipes5() was really confusing for me. Also lots of code duplication. So I parametrized these tests.
|
|
in jump.py and test_jump.py before I make actual changes
tests because it fixes a naming bug and also to prep for the next step where I add the failing test case before real fix
Co-authored-by: Brett Graham <[email protected]>
pllim
force-pushed
the
total-ellipse-of-my-points
branch
from
5d7e086 to
795daac
Compare
because its duplicate removed in another PR that got merged
and update tests
| assert result | ||
| @pytest.mark.parametrize( | ||
| ("ellipse", "point"), | ||
| [(((0, 0), (2, 4), -10), (1, 0.6)), |
There was a problem hiding this comment.
The old algorithm wrongly assumed second element is semi axis but it is actually full length, so I updated (1, 2) to (2, 4) in the input test cases for second element.
|
I reran RT. jwst still has 38 failures that need vetting. romancal has no failures. |
3 participants
Resolves JP-4174
This PR addresses
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)"stcal@git+https://github.com/pllim/stcal@total-ellipse-of-my-points")jwstregression test https://github.com/spacetelescope/RegressionTests/actions/runs/20146487129 and https://github.com/spacetelescope/RegressionTests/actions/runs/20279384974romancalregression test https://github.com/spacetelescope/RegressionTests/actions/runs/20146506582 and https://github.com/spacetelescope/RegressionTests/actions/runs/20279389429