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

Apply code style rules to wfss_contam #9151

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

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Feb 6, 2025

Partially Resolves JP-3860

This PR applies the new code style rules to the wfss_contam module.

Test coverage is sparse for this module, but the step is currently turned off by default. Additional unit testing can (IMO) wait until INS has a chance to tell us what changes they would like to see in the step - there are still several open tickets about that.

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 6, 2025

Regression tests https://github.com/spacetelescope/RegressionTests/actions/runs/13184846033

failures match the ones on main

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 13.72549% with 88 lines in your changes missing coverage. Please review.

Project coverage is 73.76%. Comparing base (717bbad) to head (95c7a02).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
jwst/wfss_contam/observations.py 6.15% 61 Missing ⚠️
jwst/wfss_contam/wfss_contam.py 0.00% 12 Missing ⚠️
jwst/wfss_contam/sens1d.py 11.11% 8 Missing ⚠️
jwst/wfss_contam/wfss_contam_step.py 25.00% 6 Missing ⚠️
jwst/wfss_contam/disperse.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9151      +/-   ##
==========================================
- Coverage   73.77%   73.76%   -0.01%     
==========================================
  Files         372      372              
  Lines       37273    37276       +3     
==========================================
  Hits        27498    27498              
- Misses       9775     9778       +3     

☔ 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 6, 2025 19:37
@emolter emolter requested review from a team as code owners February 6, 2025 19:38
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are minimal tests for this function, we should try to be as safe as possible with changes here. It looks like there's at least one probably-breaking change here, noted below. Can you please revise, with an eye to just doing the safest fixes?

Comment on lines -10 to -13
def dispersed_pixel(x0, y0, width, height, lams, flxs, order, wmin, wmax,
sens_waves, sens_resp, seg_wcs, grism_wcs, ID, naxis,
oversample_factor=2, extrapolate_sed=False, xoffset=0,
yoffset=0):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing ID from the arguments here looks a little dangerous, since this function is called via a map function in observations. It looks like you replaced ID with 0 in the parameters instead of removing it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed - this breaks the one regtest we have for wfss_contam, turned off because it's too slow even for the regtest system:
TypeError: dispersed_pixel() takes from 14 to 18 positional arguments but 19 were given

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right - I should have been more careful. I will revert the changes and wait until later to make them. The reason for them was that I realized the index didn't make any sense when I was trying to write a coherent docstring for it. But it's probably better to just mention that it's unnecessary in the docstring and leave any changes to the call signature for later, when I'm writing unit tests too.

Why doesn't the regression test run as part of the rest of the suite?

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