-
Notifications
You must be signed in to change notification settings - Fork 169
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-3777: Select first and last groups in ramp fitting #9095
base: main
Are you sure you want to change the base?
Conversation
…oups to be used in the fit Added a unit test and updated docs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9095 +/- ##
==========================================
+ Coverage 78.23% 78.49% +0.25%
==========================================
Files 505 505
Lines 46249 46852 +603
==========================================
+ Hits 36183 36776 +593
- Misses 10066 10076 +10
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than improving code coverage as @emolter suggests, it all looks good.
@@ -456,6 +458,30 @@ def process(self, step_input): | |||
|
|||
int_times = result.int_times | |||
|
|||
# Set the DO_NOT_USE bit in the groupdq values for groups before firstgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend moving this section of code to a function and calling that function conditioned on if firstgroup is not None or lastgroup is not None:
.
@@ -258,6 +258,19 @@ def test_int_times2(generate_miri_reffiles, setup_inputs): | |||
|
|||
assert len(cube_model.int_times) == nints | |||
|
|||
def test_set_groups(generate_miri_reffiles, setup_inputs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve test coverage, by creating the function above, a new test could be parametrize firstgroup
and lastgroup
and calling only the new function, ensuring code coverage during testing for various combinations of these new parameters, without having to fit ramps, testing only that the group DQ flags get set properly. Although, I still recommend keeping this test to ensure a basic ramp fit works as expected using these new parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new parametrized test that tested that the appropriate warnings are emitted
Tested and looks like this is working as expected to me; this will be a really helpful tool for instrument teams to study detector systematics in the ramps. |
Add test for user warnings (currently failing)
Added a unit test and updated docs
Resolves JP-3777
Closes #8872
This PR addresses JP-3777. It enables users to select firstgroup and lastgroup parameters when performing
ramp fitting. It works by setting the DO_NOT_USE bit in the GROUPDQ extension for groups outside the selected range.
Added a unit test and updated the docs.
Tasks
Build 11.3
(use the latest build if not sure)no-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)docs/
pageokify_regtests
to update the truth filesnews fragment change types...
changes/<PR#>.general.rst
: infrastructure or miscellaneous changechanges/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.datamodels.rst
changes/<PR#>.scripts.rst
changes/<PR#>.fits_generator.rst
changes/<PR#>.set_telescope_pointing.rst
changes/<PR#>.pipeline.rst
stage 1
changes/<PR#>.group_scale.rst
changes/<PR#>.dq_init.rst
changes/<PR#>.emicorr.rst
changes/<PR#>.saturation.rst
changes/<PR#>.ipc.rst
changes/<PR#>.firstframe.rst
changes/<PR#>.lastframe.rst
changes/<PR#>.reset.rst
changes/<PR#>.superbias.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.rscd.rst
changes/<PR#>.persistence.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.charge_migration.rst
changes/<PR#>.jump.rst
changes/<PR#>.clean_flicker_noise.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.gain_scale.rst
stage 2
changes/<PR#>.assign_wcs.rst
changes/<PR#>.badpix_selfcal.rst
changes/<PR#>.msaflagopen.rst
changes/<PR#>.nsclean.rst
changes/<PR#>.imprint.rst
changes/<PR#>.background.rst
changes/<PR#>.extract_2d.rst
changes/<PR#>.master_background.rst
changes/<PR#>.wavecorr.rst
changes/<PR#>.srctype.rst
changes/<PR#>.straylight.rst
changes/<PR#>.wfss_contam.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.fringe.rst
changes/<PR#>.pathloss.rst
changes/<PR#>.barshadow.rst
changes/<PR#>.photom.rst
changes/<PR#>.pixel_replace.rst
changes/<PR#>.resample_spec.rst
changes/<PR#>.residual_fringe.rst
changes/<PR#>.cube_build.rst
changes/<PR#>.extract_1d.rst
changes/<PR#>.resample.rst
stage 3
changes/<PR#>.assign_mtwcs.rst
changes/<PR#>.mrs_imatch.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.exp_to_source.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.tso_photometry.rst
changes/<PR#>.stack_refs.rst
changes/<PR#>.align_refs.rst
changes/<PR#>.klip.rst
changes/<PR#>.spectral_leak.rst
changes/<PR#>.source_catalog.rst
changes/<PR#>.combine_1d.rst
changes/<PR#>.ami.rst
other
changes/<PR#>.wfs_combine.rst
changes/<PR#>.white_light.rst
changes/<PR#>.cube_skymatch.rst
changes/<PR#>.engdb_tools.rst
changes/<PR#>.guider_cds.rst