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-3546: fix deprecations and improve warning filtering #8320

Merged
merged 11 commits into from
Mar 25, 2024

Conversation

emolter
Copy link
Collaborator

@emolter emolter commented Feb 29, 2024

Resolves JP-3546

Closes #8296

This PR addresses replacing all deprecated code inside jwst, and replacing all instances of warnings.simplefilter("ignore") with more specific warnings like warnings.filterwarnings("ignore", category=RuntimeWarning, message="Divide by zero"). A few other easy-to-fix warnings were also fixed or suppressed.

There were two instances of warnings.simplefilter("ignore") which, when removed, did not create any additional warnings, making it difficult to track down the warnings that the filters were intended to handle. These were rules_level2_base.py line 565, and photom.py line 476. These filters were removed; however, it's possible that they should still be there, but the test coverage was inadequate to trip the warnings.

The Jenkins run was set up such that I could convince myself that only known and understood warnings would be skipped, and the rest would raise errors. I enforced this by modifying the ini_options in pyproject.toml. These changes to pyproject.toml should be reverted before approval! See the JIRA ticket for explanations of the warnings that are being filtered. Note that the oldest deps check is failing during parsing of these options, which will be removed anyway.

Jenkins run here

All regtest failures were due to stdatamodels version issues - failures are identical to run 1248. I'm sure there will be changes requested on this PR, and I'll plan to make a clean regtest run at that time.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.32%. Comparing base (2fb073e) to head (df2b23f).
Report is 6 commits behind head on master.

❗ Current head df2b23f differs from pull request most recent head 61e90c1. Consider uploading reports for the commit 61e90c1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8320      +/-   ##
==========================================
+ Coverage   75.31%   77.32%   +2.01%     
==========================================
  Files         474      451      -23     
  Lines       38965    35089    -3876     
==========================================
- Hits        29345    27132    -2213     
+ Misses       9620     7957    -1663     
Flag Coverage Δ *Carryforward flag
nightly 77.32% <ø> (-0.02%) ⬇️ Carriedforward from 4cc0ac1

*This pull request uses carry forward flags. Click here to find out more.

☔ 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 March 1, 2024 16:36
@emolter emolter requested a review from a team as a code owner March 1, 2024 16:36
jwst/associations/lib/rules_level2_base.py Outdated Show resolved Hide resolved
jwst/datamodels/tests/test_model_container.py Show resolved Hide resolved
jwst/photom/photom.py Outdated Show resolved Hide resolved
jwst/regtest/test_nircam_coron3.py Show resolved Hide resolved
jwst/resample/resample_spec.py Show resolved Hide resolved
jwst/resample/resample_spec.py Show resolved Hide resolved
jwst/resample/resample_step.py Show resolved Hide resolved
@emolter emolter requested a review from hbushouse March 13, 2024 13:59
@nden
Copy link
Collaborator

nden commented Mar 21, 2024

The regression test run linked above shows 12 warnings. A regular one shows ~500000. Is that right?

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

After reviewing again it looks good to me. Just a couple places where I think commented out lines could be removed.

jwst/associations/lib/rules_level2_base.py Outdated Show resolved Hide resolved
jwst/photom/photom.py Outdated Show resolved Hide resolved
@emolter
Copy link
Collaborator Author

emolter commented Mar 22, 2024

The regression test run linked above shows 12 warnings. A regular one shows ~500000. Is that right?

@nden this is true, but misleading. Most warnings will return once the changes I made to the filterwarnings list in pyproject.toml are reverted. You can see the list of warning types I ignored for this PR there.

I tried to limit the scope of this PR to fixing DeprecationWarnings and to ensuring that all pre-existing calls to warnings.filterwarnings() were specific to the warning they intended to catch (so that e.g. no deprecation warnings were hidden). I did not attempt to remove all warnings in the code; this would be a much bigger project, although a few other small bits of warning-related cleanup did end up in here, or triggered by this (e.g. JP-3555)

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

LGTM

@hbushouse hbushouse merged commit bab1c7e into spacetelescope:master Mar 25, 2024
25 checks passed
@hbushouse hbushouse added this to the Build 10.2 milestone Mar 25, 2024
@emolter emolter deleted the JP-3546 branch March 25, 2024 13:02
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.

Fix deprecated code and improve existing warning filters
4 participants