Skip to content

Conversation

penaguerrero
Copy link
Contributor

@penaguerrero penaguerrero commented Apr 28, 2025

Resolves JP-3908

This PR implements STFITSDiff instead of astropy.FITSDiff to show our ad hoc reports.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (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 changelog readme for instructions)
    • 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

Copy link

codecov bot commented Apr 28, 2025

Codecov Report

❌ Patch coverage is 1.69492% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.85%. Comparing base (7ec57a3) to head (4e3e5eb).
⚠️ Report is 666 commits behind head on main.

Files with missing lines Patch % Lines
jwst/regtest/st_fitsdiff.py 0.00% 58 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9414      +/-   ##
==========================================
- Coverage   77.93%   77.85%   -0.08%     
==========================================
  Files         362      362              
  Lines       36252    36288      +36     
==========================================
  Hits        28253    28253              
- Misses       7999     8035      +36     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@penaguerrero
Copy link
Contributor Author

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.

Looks good to me. I don't see any reason why we shouldn't just update all of these at once as you suggest, IMO the new version is strictly better (or will be once any remaining kinks are ironed out).

@penaguerrero penaguerrero changed the title JP-3988: Enhance the table reports for STFITSDiff JP-3908: Incorporate stfitsdiff into regression tests Apr 30, 2025
@melanieclarke melanieclarke added this to the Build 12.0 milestone Apr 30, 2025
@melanieclarke
Copy link
Collaborator

@penaguerrero - now that the table diffs are merged, can you please run regression tests again? Then I think we should be okay to get this merged too.

@penaguerrero
Copy link
Contributor Author

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.

Looks like regtests caught a problem with the table reports we overlooked in the last PR, for non-float data. I'm going to mark this as changes-requested until we get that fixed.

@penaguerrero
Copy link
Contributor Author

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.

Thanks for the fixes! I think we're ready to give this a try.

@melanieclarke melanieclarke enabled auto-merge May 8, 2025 19:06
@melanieclarke melanieclarke merged commit 3d3e376 into spacetelescope:main May 8, 2025
19 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.

3 participants