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

RCAL-913 Resolve numpy 2 issues #1447

Conversation

WilliamJamieson
Copy link
Collaborator

Resolves RCAL-913

Closes #1413

This PR together with spacetelescope/stcal#305 (already merged) resolves all the issues from numpy 2 detected by the unit tests.

Note, I need to run the regression tests to confirm I caught all the issues.

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

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.11%. Comparing base (09e3844) to head (0e18b1f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
romancal/skymatch/region.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1447      +/-   ##
==========================================
+ Coverage   78.09%   78.11%   +0.01%     
==========================================
  Files         119      119              
  Lines        7744     7744              
==========================================
+ Hits         6048     6049       +1     
+ Misses       1696     1695       -1     
Flag Coverage Δ *Carryforward flag
nightly 62.12% <ø> (ø) Carriedforward from 09e3844

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

@schlafly
Copy link
Collaborator

That looks good. Could you say a word or two about needing to change the cross products from 2-vectors to 3-vectors? Did numpy 2 just remove 2D cross products? Thanks!

@WilliamJamieson WilliamJamieson force-pushed the feature/fix_skymatch_numpy2 branch from 1569b46 to f9d29b3 Compare October 11, 2024 14:03
@WilliamJamieson
Copy link
Collaborator Author

That looks good. Could you say a word or two about needing to change the cross products from 2-vectors to 3-vectors? Did numpy 2 just remove 2D cross products? Thanks!

It is marked as deprecated, and generates ~192000 warnings. It is depreciated as part of the lead into numpy 2 being fully python array API compliant, see numpy/numpy#24818. Cross products themselves are not really defined for products of 2D vectors in general; hence, the removal. In reality, this is just a shorthand for computing the determinant of the matrix made with the two vector. This is what the update should really be doing.

@WilliamJamieson WilliamJamieson force-pushed the feature/fix_skymatch_numpy2 branch from d2eee43 to f05c271 Compare October 14, 2024 17:31
image.meta.background.level = sky
# In numpy 2, the dtypes are more carefully controlled, so to match the
# schema the data type needs to be re-cast to float64.
image.meta.background.level = sky.astype(np.float64)
Copy link
Collaborator Author

@WilliamJamieson WilliamJamieson Oct 14, 2024

Choose a reason for hiding this comment

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

@schlafly

The tighter adherence to dtypes means that this number should have been a float32 before, but that violates the schemas: https://github.com/spacetelescope/rad/blob/b61c261deb1e21e13401e480a9790c6a212a0861/src/rad/resources/schemas/sky_background-1.0.0.yaml#L17. Should those be updated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's certainly the case that f32 is enough precision here. If we can update the schema without needing to do more than okify, then it probably makes most sense to update the rad schema to f32. But 4 bytes either way isn't a big deal, so if it's harder than running 'okify' then we should probably do the cast as you show here.

@WilliamJamieson WilliamJamieson force-pushed the feature/fix_skymatch_numpy2 branch from f05c271 to 6828d4a Compare October 14, 2024 18:59
@WilliamJamieson WilliamJamieson force-pushed the feature/fix_skymatch_numpy2 branch from c05d7aa to 36a2644 Compare October 15, 2024 17:51
@WilliamJamieson WilliamJamieson marked this pull request as ready for review October 15, 2024 17:56
@WilliamJamieson WilliamJamieson requested a review from a team as a code owner October 15, 2024 17:56
@WilliamJamieson
Copy link
Collaborator Author

These changes do not effect the regression tests when run with numpy 1, but the regression tests have slightly different results when run with numpy 2.

@schlafly
Copy link
Collaborator

This looks good to me. FWIW, I inspected the regtests and they don't vary by more than a part in 10^8 or so for the small values; the rare large reported fractional differences are due to zeros becoming 10^-8s. I did not actually try to figure out those differences---they're a little weird in that they affect only the pipeline regtests---but they are scientifically unimportant.

@WilliamJamieson WilliamJamieson merged commit 2932636 into spacetelescope:main Oct 17, 2024
31 checks passed
@WilliamJamieson WilliamJamieson deleted the feature/fix_skymatch_numpy2 branch October 17, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update elpp steps to numpy 2
2 participants