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

Update Cas22 ramp fitting to integrate with the jump detection changes in stcal #933

Conversation

WilliamJamieson
Copy link
Collaborator

@WilliamJamieson WilliamJamieson commented Oct 11, 2023

This PR is intended to only make the necessary changes in romancal to support the Cas22 algorithm's interface changes introduced by spacetelescope/stcal#215 for the purposes of integrating jump detection directly into the the ramp fit itself. This PR is not intended to add support for the new jump detection algorithm, instead it is intended to resolve the minor interface changes introduced which break romancal. A follow on PR is planned to enable and test the new jump detection algorithm in romancal.

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

@github-actions github-actions bot added ramp_fitting testing dependencies Pull requests that update a dependency file labels Oct 11, 2023
@WilliamJamieson WilliamJamieson force-pushed the feature/update_ramp_fitting branch 2 times, most recently from d93fd0f to b057a06 Compare October 25, 2023 14:07
@WilliamJamieson WilliamJamieson changed the title Enable use of jump detection algorithm that integrates into the ramp fitting Update Cas22 ramp fitting to integrate with the jump detection changes in stcal Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Files Coverage Δ
romancal/ramp_fitting/ramp_fit_step.py 32.52% <8.33%> (-1.11%) ⬇️

📢 Thoughts on this report? Let us know!.

@WilliamJamieson WilliamJamieson marked this pull request as ready for review October 25, 2023 14:25
@WilliamJamieson WilliamJamieson requested a review from a team as a code owner October 25, 2023 14:25
@schlafly
Copy link
Collaborator

This PR looks good to me, but I'm somewhat leery of having a dependency against WilliamJamieson/stcal.git---e.g., if Mairan needs to make more changes to stcal for resample, it feels like it gets really complicated to have that right. When do you think we'll be able to merge your changes into stcal?

@WilliamJamieson
Copy link
Collaborator Author

This PR looks good to me, but I'm somewhat leery of having a dependency against WilliamJamieson/stcal.git---e.g., if Mairan needs to make more changes to stcal for resample, it feels like it gets really complicated to have that right. When do you think we'll be able to merge your changes into stcal?

If you look at the commit history those changes are all in a single commit with a message indicating they are temporary. The reason for them is that this PR and spacetelescope/stcal#215 are interlinked due to the interface change. Since romancal already directly depends on stcal's development version rather than a published stable version, merging spacetelescope/stcal#215 will immediately break romancal without the changes in this PR. However, the changes for this PR require the changes in spacetelescope/stcal#215 in order to function. As a consequence both PRs need to be merged in short order i.e. merge spacetelescope/stcal#215 -> remove the temporary commit -> merge this PR so that there are minimal disruptions to the actual functionality of romancal.

All this is to say, both this PR and spacetelescope/stcal#215 need to be fully reviewed and "ready" before either can be merged. The temporary commit is just acting so that these changes can be fully tested and reviewed as it places both codebases in the "final" configuration needed.

@schlafly
Copy link
Collaborator

Sounds good, except for the dependency I'm happy to sign off on these changes.

@WilliamJamieson WilliamJamieson force-pushed the feature/update_ramp_fitting branch from b057a06 to 6fb3154 Compare October 26, 2023 15:35
@WilliamJamieson WilliamJamieson force-pushed the feature/update_ramp_fitting branch from 6fb3154 to 9e9822f Compare October 26, 2023 15:55
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Oct 26, 2023
@schlafly
Copy link
Collaborator

Tests are failing because this points to stcal/main, and spacetelescope/stcal#215 hasn't been merged yet. I saw that tests were successful when this was pointed to the correct PR. I'm approving this, with the understanding that we won't actually merge it until 215 is in.

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.

Looks good; tests failing due to unmerged stcal PR 215 are understood. Good to go in when that is merged.

@WilliamJamieson
Copy link
Collaborator Author

Note that due to tiny numerical differences the ramp_fitting regression test will start failing. However, @schlafly and I have reviewed and discussed the changes offline and determined that these are minor (due to a single unrealistic pixel in the regression data), and thus can be ok-ified after the next regression test run.

@WilliamJamieson WilliamJamieson merged commit 34bead0 into spacetelescope:main Oct 26, 2023
23 of 24 checks passed
@WilliamJamieson WilliamJamieson deleted the feature/update_ramp_fitting branch October 26, 2023 17:20
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants