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

RCAL-513 Update uneven ramp fitting documentation #944

Merged
merged 19 commits into from
Oct 26, 2023

Conversation

stscieisenhamer
Copy link
Collaborator

@stscieisenhamer stscieisenhamer commented Oct 18, 2023

Resolves RCAL-513

Closes #

This PR addresses ...

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@stscieisenhamer stscieisenhamer added documentation Improvements or additions to documentation ramp_fitting labels Oct 18, 2023
@stscieisenhamer stscieisenhamer self-assigned this Oct 18, 2023
@stscieisenhamer
Copy link
Collaborator Author

@schlafly Still work-in-progress. However, I wanted to get a review of the description section. I have added the information that is in the paper and should correspond to the stcal.ramp_fitting.ols_cas22.fit_ramps code.

However, there is a bit more math, going from the segments to the resultants, that occurs in stcal.ramp_fitting.ols_cas22.fit_ramps_casertano after the fit_ramps is called, basically combining the segments back to their resultants. I can "math this out", but was wondering if there is a source paper (or something) that is better to use to match terminology.

There are still technical issues, such as filling out the parameters for the step, etc. that need to be completed, but wanted you to get an early look at this.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
@schlafly
Copy link
Collaborator

This looks good to me. Re combining segment ramps into a single ramp for each pixel, the goal was to do an inverse-variance weighted average using the read noise variances for each segment. That's not part of the Casertano+22 paper, as you say, and isn't documented anywhere.

Generically the right thing to do when combining a bunch of measurements to get a mean is to do an inverse variance weighted average with the total variances. However, the Poisson variance is proportional to the derived slope, so if you use that to do the weighting you systematically downweight ramps where the flux is high, leading to a negative bias. It would likely be better to do something like derive an initial flux from a straight average or median of the segments, use it to compute new total variances for each segment, and then use those total variances to do the inverse variance weighted mean. But that could be future work; I just did the inverse variance weighted mean using the read noise variances.

@stscieisenhamer stscieisenhamer changed the title RCAL-513 Uneven ramp fitting documentation RCAL-513 Update uneven ramp fitting documentation Oct 23, 2023
@stscieisenhamer stscieisenhamer marked this pull request as ready for review October 23, 2023 04:45
@stscieisenhamer stscieisenhamer requested a review from a team as a code owner October 23, 2023 04:45
@stscieisenhamer
Copy link
Collaborator Author

stscieisenhamer commented Oct 23, 2023

Rest of the formulae have been documented. This will definitely need re-review. Also updated the parameter documentation.

@stscieisenhamer
Copy link
Collaborator Author

Regression test though it appears something is up with them, and this is documentation only anyways.

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This looks good. I left a few minor comments. The most important one is to check the denominator of the variances; I think some parentheses are misplaced. Thanks!

docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
docs/roman/ramp_fitting/description.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Sorry, a few more minor nits, but looks good!

-----------------------------

The segment fitting implementation is based on Section 5 of Casertano et.
al. 2022. If there is only one segment, no fitting is performed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think of a 'segment' as a block of consecutive resultants with no jumps in between. One segment is fine; e.g., this is the case for a ramp with no cosmic rays. A segment with one resultant isn't fine and gets skipped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, I got the terminology wrong. Trying again...

Comment on lines 34 to 36
terminated where saturation flags are found. Pixels are processed simultaneously
in blocks. The size of the block depends on the image size and the number of
resultants.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
terminated where saturation flags are found. Pixels are processed simultaneously
in blocks. The size of the block depends on the image size and the number of
resultants.
terminated where saturation flags are found.

We just process pixels one by one.

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Changes
- update index to include ramp fitting
- reorder index to be alphabetical
Changes:
- move OLS to its own file
- update links
- prepare main description for ols_cas22
Changes:
- Remove content particular to the OLS algorithm
- Update weighting for ols_cass22
@stscieisenhamer stscieisenhamer merged commit 10e90bd into spacetelescope:main Oct 26, 2023
24 checks passed
ddavis-stsci pushed a commit to ddavis-stsci/romancal that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ramp_fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants