-
Notifications
You must be signed in to change notification settings - Fork 167
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-3784: Fix off-nominal pixel_replace and cube_build output names #9019
base: main
Are you sure you want to change the base?
JP-3784: Fix off-nominal pixel_replace and cube_build output names #9019
Conversation
Regression tests running here: Test shows no failures. |
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.
LGTM, I like the improvements to test_invariant_filename
. Admittedly I only skimmed this because I assume it is similar to Jane's PR that I already looked at
@emolter - this is the updated fix for the pixel_replace output names. I will leave it at draft so it doesn't get merged for 11.2, but it is ready for review. It turns out the only fix necessary was to default the pixel replace step to output_use_model=True. This is done for several other similar steps where multiple input are used, but output files should be based on the input name (tweakreg, master_background, cube_build), and should probably be added anywhere we might reasonably expect the input and output to be a list of files. I kept the change to put invariant_filename into stpipe.utilities, because that seems like the right place for it anyway, but it is no longer needed to fix the filenames. |
d8a781a
to
095d364
Compare
So to be clear, this is an stpipe option available to all steps, and it's added to the spec just because we want to change the default value? It seems to me we might reasonably expect a list of files in any step that has a container as output. Are you thinking of any other steps in particular? |
Yes, the default value is False. We might want to consider whether the default value should be True, because all stpipe steps will generate similar nonsense names if the model to be saved is a Sequence.
I don't know for sure if there are any other steps that need this. The only specific step I was wondering about was outlier_detection, which does not modify this parameter. Looking at it now, though, it looks like it always returns a ModelLibrary, so it probably isn't affected - ModelLibraries are saved by iterating over the models. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9019 +/- ##
==========================================
+ Coverage 76.81% 76.89% +0.08%
==========================================
Files 496 497 +1
Lines 45610 45656 +46
==========================================
+ Hits 35034 35108 +74
+ Misses 10576 10548 -28
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Resolves JP-3784
Fixes output filenames for pixel_replace and cube_build when the steps are called outside the pipelines. For pixel_replace, only the case where the input is a ModelContainer is affected.
Based on #8979, with a few additional fixes and test updates.
This PR also includes a minor refactor to move the
invariant_filename
function from spec3 to stpipe.utilities. In the end, that function wasn't needed to fix this problem, but it still seems like a more useful place for that function to be stored.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