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-3896: Deprecate or remove various assign_wcs utility functions #9221

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Feb 25, 2025

Resolves JP-3896

Closes #9218

This PR removes various unused utility functions from assign_wcs, as follows:

  • _domain_to_bounding_box: unused, and private
  • reproject: part of __all__ of assign_wcs.util but unused elsewhere in the pipeline, and not in the documentation (because not part of __all__ in the top-level step). There are also multiple functions of the same name in stcal with similar (identical?) functionality. Because this was in the __all__ though, I have deprecated it here instead of removing right away.
  • compute_fiducial: Only used inside a test of the resample step, and the test was easy to modify such that this was not needed.
  • is_fits: unused
  • get_num_msa_open_shutters: unused

Before this is merged, we need to know if any of these are used outside the pipeline. If they are, they should be added to the __all__ of the top-level step so their API renders in readthedocs, and if not, we can safely remove them.

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

@emolter
Copy link
Collaborator Author

emolter commented Feb 25, 2025

regression tests started here: https://github.com/spacetelescope/RegressionTests/actions/runs/13525309517

edit: all passing

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.72%. Comparing base (16aa8b0) to head (5e44593).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
jwst/assign_wcs/util.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9221      +/-   ##
==========================================
+ Coverage   73.66%   73.72%   +0.05%     
==========================================
  Files         368      368              
  Lines       36392    36331      -61     
==========================================
- Hits        26808    26784      -24     
+ Misses       9584     9547      -37     

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

@emolter emolter marked this pull request as ready for review February 25, 2025 17:12
@emolter emolter requested review from a team as code owners February 25, 2025 17:12
@emolter emolter requested review from nden and drlaw1558 February 25, 2025 17:12
@emolter
Copy link
Collaborator Author

emolter commented Feb 25, 2025

@nden @drlaw1558 I thought you two would be the people most likely to know whether any of these are being used anywhere outside the pipeline.

@drlaw1558
Copy link
Collaborator

@emolter I'm not aware of any such use beyond the pipeline; I can bounce it off the instrument teams though.

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.

Clean up WCS utility functions
3 participants