Skip to content

Conversation

@kmacdonald-stsci
Copy link
Collaborator

Resolves JP-3164

This PR cleans up CI test removing or updating skipped or xfail'd tests.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (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)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@codecov
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

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

Project coverage is 89.74%. Comparing base (6167e76) to head (b1247e3).
Report is 103 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/saturation/saturation.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   88.02%   89.74%   +1.71%     
==========================================
  Files          64       64              
  Lines        9983     9965      -18     
==========================================
+ Hits         8788     8943     +155     
+ Misses       1195     1022     -173     

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


# Make sure that groups after the third get flagged
assert np.all(gdq[0, 2:, 5, 5] == DQFLAGS["SATURATED"])
assert np.all(gdq[0, 3:, 5, 5] == DQFLAGS["SATURATED"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the behavior introduced by #321 is not desirable long term; if we opt to remove the xfail here, we should at least add a comment describing the ideal state for future edits to the code and this test. I believe the long-term fix will be tracked under https://jira.stsci.edu/browse/JP-3835

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.

Overall these look good to me Ken, but what is the status of

@pytest.mark.skip(reason="Getting all NaN's, but expecting all zeros.")
? Are you well-versed enough in ramp fitting to take a look at that one and add it to this PR too, or should we try to get someone else to do it?

pllim added a commit to pllim/stcal that referenced this pull request Dec 15, 2025
and fix tests.

MNT: Add __all__ to ramp_fitting/utils.py

DOC: Fix DQ propagation text.

TST: Un-xfail test_read_pattern_saturation_flagging from spacetelescope#362
assert np.all(gdq[0, 2:, 5, 5] == DQFLAGS["SATURATED"])
# XXX - PR #321 introduced this behavior, but it may not be what's wanted. For now
# the xfail has been removed to test the current behavior.
assert np.all(gdq[0, 3:, 5, 5] == DQFLAGS["SATURATED"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I caught this one separately at #421 so I copied this part of the PR over to my PR there. FYI.

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.

4 participants