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-1887: Refactoring ols_ramp_fit into smaller more manageable functions #5819

Merged
merged 6 commits into from
Mar 10, 2021

Conversation

kmacdonald-stsci
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci commented Mar 8, 2021

Addresses #5701 / JP-1887

This is one in a series of PR's designed to refactor ramp fitting to make it more readable, maintainable, and reusable.

This PR is used to break down the function ols_ramp_fit. It is over 700 lines long, but has four distinct steps, which have been encapsulated in four different functions that ols_ramp_fit calls.

  • ramp_fit_miri_multigroups(), deals with the special case of MIRI data with multiples groups in an integration, resizing the data for discarded groups.
  • ramp_fit_first_pass(), does the "first pass" over the data to compute the initial values related to slope computation.
  • ramp_fit_second_pass(), does the "second pass" over the data using the intermediate results from the first pass to compute values related to variances of estimates.
  • ramp_fit_overall_slopes_and_variances(), uses the values computed in the first and second passes over the date to compute the overall slopes and variances for each pixel.

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #5819 (c3d3587) into master (a46c4fd) will increase coverage by 1.08%.
The diff coverage is 72.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5819      +/-   ##
==========================================
+ Coverage   75.66%   76.74%   +1.08%     
==========================================
  Files         406      406              
  Lines       36323    37965    +1642     
==========================================
+ Hits        27484    29137    +1653     
+ Misses       8839     8828      -11     
Flag Coverage Δ *Carryforward flag
nightly 76.63% <68.47%> (+0.96%) ⬆️ Carriedforward from c8f52da
unit 54.54% <82.05%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
jwst/ramp_fitting/ramp_fit.py 82.32% <72.88%> (+2.77%) ⬆️
jwst/skymatch/region.py 74.42% <0.00%> (-6.37%) ⬇️
jwst/ami/utils.py 73.48% <0.00%> (-4.78%) ⬇️
jwst/resample/resample.py 92.30% <0.00%> (-3.89%) ⬇️
jwst/rscd/rscd_sub.py 41.54% <0.00%> (-3.40%) ⬇️
jwst/ami/analyticnrm2.py 82.83% <0.00%> (-2.75%) ⬇️
jwst/associations/lib/rules_level3.py 97.53% <0.00%> (-1.71%) ⬇️
jwst/ramp_fitting/create_cube.py 6.48% <0.00%> (+0.28%) ⬆️
jwst/cube_build/cube_build_step.py 66.50% <0.00%> (+0.94%) ⬆️
jwst/assign_wcs/util.py 80.00% <0.00%> (+1.11%) ⬆️
... and 11 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 a46c4fd...c3d3587. Read the comment docs.

@jdavies-st jdavies-st self-requested a review March 9, 2021 16:08
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 Ken. Just a couple questions below.

nreads: int
Number of reads

ngroups: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable groups is gotten from input_model.meta.exposure.ngroups in the RampModel passed to ramp_fit. It is not part of the shape of data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Be very careful with the use and meaning of nreads vs ngroups. In the instruments, individual detector reads can be averaged on-board and then downlinked as a single group. When the data hit the ground they are 4-dimensional: [ncols, nrows, ngroups, nints] in conventional (e.g. FITS) notation, which is [nints, ngroups, nrows, ncols] in python array notation. The data in the ngroups dimension can consist of 1 or more detector reads. So in this particular use in the ramp_fit step I'm guessing nreads refers to the actual number of detector readouts that took place in the instrument, which is not necessarily the same as ngroups (because, as mentioned above, 1 or more reads can get averaged into a group). So nreads can't reliably be determined from the shape of the 4-D data array. Only ngroups can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon clarification, the use of nreads and ngroups are the same values taken from two different locations in the RampModel class. in effect, these two variables are redundant that makes the code confusing. I will remove this redundancy and use the correct ngroups variable name.

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.

Cool. I think this is good to go then as is.

@jdavies-st jdavies-st changed the title Jp 1887: Refactoring ols_ramp_fit into smaller more manageable functions JP-1887: Refactoring ols_ramp_fit into smaller more manageable functions Mar 10, 2021
@jdavies-st jdavies-st added this to the Build 7.8 milestone Mar 10, 2021
@jdavies-st
Copy link
Collaborator

Ran the regtests on this PR and all pass. I think this is good to merge.

@jdavies-st jdavies-st merged commit c816060 into spacetelescope:master Mar 10, 2021
@kmacdonald-stsci kmacdonald-stsci deleted the jp_1887_refac_2 branch March 11, 2021 00:46
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.

3 participants