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

Removed redundant calculations, unneeded array allocations from jump step #302

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

t-brandt
Copy link

@t-brandt t-brandt commented Oct 8, 2024

Resolves JP-3697

Closes #

This PR affects the two-point-difference jump step only. It removes duplicate and redundant calculations and unneeded array allocations, which make a significant difference for large uncal files. Masked array median is also changed to nanmedian, and a few variables are renamed to avoid overwriting a quantity that is needed at the end of the routine.

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

@t-brandt t-brandt requested a review from a team as a code owner October 8, 2024 17:51
@github-actions github-actions bot added the jump label Oct 8, 2024
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 10 lines in your changes missing coverage. Please review.

Project coverage is 86.89%. Comparing base (35726e9) to head (412f078).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/jump/twopoint_difference.py 85.71% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #302       +/-   ##
===========================================
+ Coverage   29.57%   86.89%   +57.31%     
===========================================
  Files          36       49       +13     
  Lines        7949     8949     +1000     
===========================================
+ Hits         2351     7776     +5425     
+ Misses       5598     1173     -4425     

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

Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

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

There are some down stream test failures. Are those related to this PR? If not, this looks fine.

@ddavis-stsci
Copy link
Collaborator

The romancal tests for these changes pass,
https://github.com/spacetelescope/RegressionTests/actions/runs/11328631054

@melanieclarke
Copy link
Contributor

melanieclarke commented Nov 4, 2024

JWST regtests are running here:
https://github.com/spacetelescope/RegressionTests/actions/runs/11672280970

Failures are unrelated (from spacetelescope/jwst#8893).

Copy link
Contributor

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

These changes look pretty straightforward, and seem worth merging as is, before we do any more profiling or refactoring for jump.

One quick question about the change to the flag handling, below. Also, the change note needs to be moved out of the CHANGES.rst file to a newer style towncrier note -- see the PR template description for the new method. That should fix the readthedocs error and the changelog check.

src/stcal/jump/twopoint_difference.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@melanieclarke
Copy link
Contributor

@t-brandt - just checking in. I think this PR just needs some minor updates and it should be ready to go. Do you want to make the changes, or would you like me to do it for you?

@t-brandt
Copy link
Author

@melanieclarke Sorry for dropping the ball on this for a while. If you have the time and bandwidth to make the minor changes, please go ahead. Otherwise I'll try to do so over the weekend or next week.

@melanieclarke
Copy link
Contributor

melanieclarke commented Dec 13, 2024

@melanieclarke
Copy link
Contributor

Romancal tests look fine. There are some failures in jwst unit and regression tests that are related to some ongoing gwcs work. I will let that settle and rerun before we merge this.

@t-brandt
Copy link
Author

I actually want to make a few more changes--I can squeeze a significant amount of additional speedup while shortening the code. I will push those changes tonight or tomorrow. We can work on merging this after 11.2 :-)

@melanieclarke
Copy link
Contributor

We can work on merging this after 11.2 :-)

Sounds good, thanks. Can you please mark this draft for now, and let us know when you're ready?

@t-brandt t-brandt marked this pull request as draft December 18, 2024 16:28
@t-brandt
Copy link
Author

@melanieclarke: I updated the PR with new changes. The new changes are more significant so I think a bit more testing/more eyeballs are warranted, which presumably will wait until after 11.2 is done. I checked that the generated gdq arrays were identical for a few test files. Memory improvements are modest, speed improvements are large, and crashes/virtual memory/very slow performance when overflagging jumps should all be gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants