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-3784: Fix off-nominal pixel_replace and cube_build output names #9019

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Dec 18, 2024

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

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<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

@melanieclarke melanieclarke added this to the Build 11.3 milestone Dec 18, 2024
@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Dec 18, 2024

Regression tests running here:
https://github.com/spacetelescope/RegressionTests/actions/runs/12401830665

Test shows no failures.

Copy link
Collaborator

@emolter emolter left a 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

jwst/pixel_replace/pixel_replace_step.py Outdated Show resolved Hide resolved
jwst/pixel_replace/pixel_replace_step.py Outdated Show resolved Hide resolved
@melanieclarke
Copy link
Collaborator Author

@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.

@emolter
Copy link
Collaborator

emolter commented Dec 19, 2024

It turns out the only fix necessary was to default the pixel replace step to output_use_model=True.

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?

@melanieclarke
Copy link
Collaborator Author

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?

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.

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?

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.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.89%. Comparing base (e3d263f) to head (095d364).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
jwst/stpipe/utilities.py 85.71% 2 Missing ⚠️
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     
Flag Coverage Δ *Carryforward flag
nightly 77.40% <ø> (-0.03%) ⬇️ Carriedforward from e3d263f

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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