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

Fix unit tests due to incoming changes in stcal #1584

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

mcara
Copy link
Member

@mcara mcara commented Jan 17, 2025

This PR fixes unit test failures that will occur if spacetelescope/stcal#326 is accepted.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (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#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.73%. Comparing base (f538ba0) to head (eee1596).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1584      +/-   ##
==========================================
- Coverage   77.94%   76.73%   -1.21%     
==========================================
  Files         115      115              
  Lines        7622     7622              
==========================================
- Hits         5941     5849      -92     
- Misses       1681     1773      +92     

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

@mcara
Copy link
Member Author

mcara commented Jan 17, 2025

I do not see how failures are related to this PR (they seem to occur in the ramp filling).

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This looks good. I don't understand the test failures, though; they are clearly unrelated to this PR, but also I don't see them failing on main. Would a rebase solve this?

def get_resampled_wcs_pixel_scale(wcs):
t = wcs.forward_transform
for p in t.param_names:
if p.startswith("factor"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

One worries a bit about this breaking in the future. I don't immediately expect other bits that would go very early in the WCS and would have names that start with factor, but it's worth at least thinking about whether alternatives exist?

@mcara mcara force-pushed the fix-footprint-unit-tests branch 2 times, most recently from 488854d to 5fb951d Compare January 21, 2025 18:59
@schlafly
Copy link
Collaborator

I'm merging this now as the failures are clearly unrelated and this is blocking stcal / jwst work.

@schlafly schlafly merged commit 64f6ead into spacetelescope:main Jan 21, 2025
24 of 30 checks passed
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