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

Weight threshold part duex #319

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Nov 14, 2024

This is "take 2" of #312

At the moment this PR will create small numerical differences in the output of compute_weight_threshold but I'm convinced that the current behavior on main is incorrect and a by-product of related numpy bugs:

A minimal example is:

import numpy as np

one_f = np.float32(1)
s_f = np.spacing(one_f)
arr_f = np.array([one_f, one_f + s_f, one_f + s_f + s_f], dtype=np.float32)

one_d = np.float64(1)
arr_d = np.array([one_d, one_d + s_f, one_d + s_f + s_f], dtype=np.float64)

print(f"expected mean: {np.mean(arr_d)=}")
print(f"{np.mean(np.ma.array(arr_f, mask=False))=}")
print(f"{np.mean(arr_f, dtype='f8')=}")

Produces:

expected mean: np.mean(arr_d)=1.0000001192092896
np.mean(np.ma.array(arr_f, mask=False))=1.0000000794728596
np.mean(arr_f, dtype='f8')=1.0000001192092896

The second line is similar to what's occuring on main where a mean of a masked array of float32 values is producing a float64 that is incorrect (given the precision). The third line is similar to what is produced with this PR.

One added benefit is this PR appears to reduce the memory usage lower than #312 (as seen by the 0.9 vs 1.9 multiplier in the memory usage test). I believe this is due to providing a non-masked array to sigma clip to avoid the allocation here:
https://github.com/astropy/astropy/blob/3bd8b404bf558561da469128d570280f3b65e57f/astropy/stats/sigma_clipping.py#L400-L401

Further optimizations might require changes to sigma clip (which appears to create an array copy for _compute_bounds:
Screenshot 2024-11-14 at 1 34 03 PM

Regression tests running:
Romancal: https://github.com/spacetelescope/RegressionTests/actions/runs/11956099949
JWST: https://github.com/spacetelescope/RegressionTests/actions/runs/11956106816
(these are expected to have a few minor differences)

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

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.68%. Comparing base (d953b16) to head (26836b8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   86.39%   86.68%   +0.28%     
==========================================
  Files          49       49              
  Lines        8902     8937      +35     
==========================================
+ Hits         7691     7747      +56     
+ Misses       1211     1190      -21     

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


🚨 Try these New Features:

@braingram braingram force-pushed the weight_threshold_part_duex branch from f16d1e9 to 38289f1 Compare November 21, 2024 15:09
@braingram braingram marked this pull request as ready for review November 21, 2024 15:13
@braingram braingram requested a review from a team as a code owner November 21, 2024 15:13
Copy link
Collaborator

@mairanteodoro mairanteodoro 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!

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. I re-ran the memory usage profiling that I used for the original PR and it shows that the memory savings are indeed working as intended.

@braingram
Copy link
Collaborator Author

Regtests started for okifying. Once these finish I'll merge then okify:
JWST https://github.com/spacetelescope/RegressionTests/actions/runs/12012419212
romancal https://github.com/spacetelescope/RegressionTests/actions/runs/12012422905
actually, romancal doesn't have a dev stcal requirement. I'll open a PR for that.

@braingram braingram merged commit d0844b5 into spacetelescope:main Nov 25, 2024
26 checks passed
@braingram braingram deleted the weight_threshold_part_duex branch November 25, 2024 16:50
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.

5 participants