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-1950: Update ramp fit unit tests #6038

Merged
merged 3 commits into from
May 20, 2021

Conversation

kmacdonald-stsci
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci commented May 18, 2021

Closes #3879
Resolves JP-1950

Description

This PR addresses unit tests for ramp fitting. A bug fix was found in the computation of the median first differences of a pixel ramp. Correcting this bug means revising unit and regression tests. The bug fix effected two unit test cases. These tests have been updated to account for the bug fix. Work needs to be done to correct regression tests.

Bugfix in spacetelescope/stcal#12

Checklist

  • Tests
  • Documentation
  • Change log
  • Milestone
  • Label(s)

Copy link
Contributor

@dmggh dmggh left a comment

Choose a reason for hiding this comment

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

Looks good to me. Does 'Work needs to be done to correct regression tests.' merely mean the truth files need to be updated ?

@kmacdonald-stsci
Copy link
Contributor Author

Yes. I should have made that more clear. The slope estimate change done in this PR effects many calculations. The regression test truth files need to be adjusted to account for this change.

@jdavies-st jdavies-st changed the title JP-1950: Updating test cases for the bug fix done for JP-1950. JP-1950: Update ramp fit unit tests May 19, 2021
@jdavies-st jdavies-st closed this May 20, 2021
@jdavies-st jdavies-st reopened this May 20, 2021
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #6038 (e2de9cf) into master (9df6401) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6038   +/-   ##
=======================================
  Coverage   77.10%   77.10%           
=======================================
  Files         401      401           
  Lines       34307    34307           
=======================================
  Hits        26454    26454           
  Misses       7853     7853           
Flag Coverage Δ *Carryforward flag
nightly 77.11% <ø> (ø) Carriedforward from 9df6401
unit 55.15% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df6401...e2de9cf. Read the comment docs.

Copy link
Collaborator

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks good. Just need to update setup.cfg and requirements-sdp.txt to have the new version. >= in the first, == in the 2nd. Then all the tests will pass.

@jdavies-st jdavies-st added this to the Build 7.8 milestone May 20, 2021
Copy link
Collaborator

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks good. Merge when tests pass.

@jdavies-st jdavies-st merged commit bfe9d64 into spacetelescope:master May 20, 2021
@kmacdonald-stsci kmacdonald-stsci deleted the jp_1950_new branch May 20, 2021 17:56
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.

GroupDQ index bug in ramp_fit.py
3 participants