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

AL-479: Move ramp fit code from JWST to STCAL. #6023

Merged
merged 3 commits into from
May 18, 2021

Conversation

kmacdonald-stsci
Copy link
Contributor

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

Resolves AL-479

Description

Moved ramp fitting code from JWST to STCAL. All unit tests and regression tests are passing, using ramp fitting path to STCAL.

Checklist

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

@kmacdonald-stsci kmacdonald-stsci changed the title AL-479; Move ramp fit code from JWST and moved it to STCAL. AL-479: Move ramp fit code from JWST and moved it to STCAL. May 13, 2021
@jdavies-st jdavies-st added this to the Build 7.8 milestone May 14, 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. Regression tests on this branch and the one on stcal look good:

https://plwishmaster.stsci.edu:8081/job/RT/job/jdavies-dev/241/

This PR depends on spacetelescope/stcal#6 being merged and a release of stcal happening. This PR should then be updated to specify that version or greater of stcal in setup.cfg. Then it can be merged.

🚀

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #6023 (d0093b0) into master (aed4cd4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head d0093b0 differs from pull request most recent head 523ef17. Consider uploading reports for the commit 523ef17 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6023      +/-   ##
==========================================
+ Coverage   77.53%   77.55%   +0.01%     
==========================================
  Files         405      405              
  Lines       35174    35220      +46     
==========================================
+ Hits        27272    27314      +42     
- Misses       7902     7906       +4     
Flag Coverage Δ *Carryforward flag
nightly 77.53% <100.00%> (ø) Carriedforward from aed4cd4
unit 55.35% <100.00%> (-1.31%) ⬇️

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

Impacted Files Coverage Δ
jwst/ramp_fitting/ramp_fit_step.py 91.79% <100.00%> (-0.26%) ⬇️

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 221273a...523ef17. Read the comment docs.

@jdavies-st jdavies-st changed the title AL-479: Move ramp fit code from JWST and moved it to STCAL. AL-479: Move ramp fit code from JWST to STCAL. May 14, 2021
@nden
Copy link
Collaborator

nden commented May 17, 2021

spacetelescope/stcal#6 was merged and I pointed this branch to stcal/main. Once tests pass we need an stcal release and can merge this one.

@nden nden requested a review from hbushouse May 17, 2021 21:30
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

lgtm

@nden
Copy link
Collaborator

nden commented May 18, 2021

stcal 0.2.0 was released on PyPi

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

So the various JWST instrument-specific hooks for special handling of a few things like MIRI first and last frames, etc. are now all in the supposedly "common" code over in stcal?

@nden
Copy link
Collaborator

nden commented May 18, 2021

They are because the common code works on data models.

@kmacdonald-stsci
Copy link
Contributor Author

kmacdonald-stsci commented May 18, 2021

I am happy to remove it from STCAL and put it into JWST. There is only one specific MIRI reference in the STCAL code, so moving it should be easy.

Are there other parts of the ramp fit code that are JWST specific that should be removed from the ramp fitting code in STCAL?

@nden nden force-pushed the al_479_remove_ramp_fit branch from d0093b0 to 7540c7d Compare May 18, 2021 12:44
@nden nden force-pushed the al_479_remove_ramp_fit branch from 7540c7d to 523ef17 Compare May 18, 2021 12:46
@nden
Copy link
Collaborator

nden commented May 18, 2021

This now points to the stcal release and can be merged.

@hbushouse
Copy link
Collaborator

I am happy to remove it from STCAL and put it into JWST. There is only one specific MIRI reference in the STCAL code, so moving it should be easy.

Are there other parts of the ramp fit code that are JWST specific that should be removed from the ramp fitting code in STCAL?

No need to unmove any of the code. I was just curious. I guess the assumption is that Roman will use the same RampModel and ImageModel models?

@kmacdonald-stsci
Copy link
Contributor Author

kmacdonald-stsci commented May 18, 2021

The assumption is that both pipelines will only use a JWST-like input RampModel containing expected values and kewords. The model input variable to ramp_fit is the only variable that depends is a data model. I've removed usage of data models everywhere else in the code. All returned values from the ramp fitting code in STCAL returns tuples of computed arrays the pipeline step code is then responsible for putting into their respective data models, like an ImageModel.

@nden nden merged commit 85feb11 into spacetelescope:master May 18, 2021
@dmggh
Copy link
Contributor

dmggh commented May 18, 2021

I think that this may have already been discussed, but as this is a change in the design and code structure (and the first of these changes among several calibration steps) this should be described in the docs for ramp fitting. The restructuring to use common code should probably also be described in other, more general sections of the docs.

@nden
Copy link
Collaborator

nden commented May 18, 2021

@dmggh good point and agreed!

@kmacdonald-stsci kmacdonald-stsci deleted the al_479_remove_ramp_fit branch May 19, 2021 13:50
jdavies-st added a commit to jdavies-st/jwst that referenced this pull request Jun 4, 2021
jdavies-st added a commit to jdavies-st/jwst that referenced this pull request Jun 7, 2021
jdavies-st added a commit that referenced this pull request Jun 7, 2021
* Revert "AL-479: Move ramp fit code from JWST to STCAL.  (#6023)"

This reverts commit 85feb11.

* Revert "AL-479: Preparing ramp fit code for removal from JWST and moved to STCAL (#6010)"

This reverts commit 5a9aaf8.

* Turn on ramp_fit multiprocessing tests

* Update CHANGES.rst
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.

5 participants