Skip to content
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

JP-3734: Making OLS_C Default for CI Tests #289

Closed

Conversation

kmacdonald-stsci
Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci commented Sep 23, 2024

…ith CHARGELOSS.

Resolves JP-3734

Closes #

The JWST default algorithm was changed to be OLS_C for ramp fitting. The STCAL ramp fit testing has been changed to reflect this change in default algorithm.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

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

Project coverage is 73.79%. Comparing base (5f94030) to head (2be0844).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/ramp_fitting/ramp_fit.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #289       +/-   ##
===========================================
- Coverage   85.19%   73.79%   -11.41%     
===========================================
  Files          46       46               
  Lines        8804     8809        +5     
===========================================
- Hits         7501     6501     -1000     
- Misses       1303     2308     +1005     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me! One very small clean up request.

Did you run regression tests somewhere?

src/stcal/ramp_fitting/ramp_fit_class.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the romancal regression tests pass with this in place.
https://github.com/spacetelescope/RegressionTests/actions/runs/11034593835

@melanieclarke
Copy link
Contributor

@kmacdonald-stsci - did regression tests get run for JWST for this change?

@kmacdonald-stsci
Copy link
Collaborator Author

@kmacdonald-stsci - did regression tests get run for JWST for this change?

Yes. None of the failures are related to any ramp fit tests. @tapastro verified the failures were due to another issue.

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1742/#showFailuresLink

@tapastro
Copy link
Collaborator

tapastro commented Oct 3, 2024

It looks like this is breaking a unit test:

FAILED tests/test_ramp_fitting.py::test_neg_med_rates_single_integration_multi_segment_optional - AssertionError: 
  Not equal to tolerance rtol=1e-06, atol=0
  
  -inf location mismatch:
   ACTUAL: array([ 0.     , 60.53071,     -inf], dtype=float32)
   DESIRED: array([0., 0., 0.])

We should probably resolve this failure before merging.

@kmacdonald-stsci
Copy link
Collaborator Author

It looks like this is breaking a unit test:

FAILED tests/test_ramp_fitting.py::test_neg_med_rates_single_integration_multi_segment_optional - AssertionError: 
  Not equal to tolerance rtol=1e-06, atol=0
  
  -inf location mismatch:
   ACTUAL: array([ 0.     , 60.53071,     -inf], dtype=float32)
   DESIRED: array([0., 0., 0.])

We should probably resolve this failure before merging.

It's not clear to me this is caused by this PR. And I've never seen such a strange problem like this. If I run pytest tests/test_ramp_fitting.py the output to be checked for this test is different that if I run pytest tests/test_ramp_fitting.py::test_neg_med_rates_single_integration_multi_segment_optional.

It doesn't matter if I run with the OLS algorithm or the OLS_C algorithm. The screenshots of the test outputs show that the returned array is all zero when running the whole test script, but is non-zero if only the specific test is run, regardless of the choice of algorithm. Because this difference occurs independent of which algorithm is chosen, I don't think this PR can be the problem.

I'll continue to investigate.

Screenshot 2024-10-03 at 3 08 59 PM Screenshot 2024-10-03 at 3 20 23 PM

@kmacdonald-stsci
Copy link
Collaborator Author

Closing this PR. This approach has introduced a subtle bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants