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

Speed up Jump detection #229

Merged

Conversation

WilliamJamieson
Copy link
Contributor

@WilliamJamieson WilliamJamieson commented Nov 9, 2023

This PR dramatically speeds up Roman's jump detection (and ramp fitting to a lesser extent). Empirical testing locally showed me a speed up from ~33sec to ~6sec for fitting a test data set using jump detection.

The main culprit for this problem was the fact that to use numpy one has to jump out of the C code temporally. This overhead was orders of magnitude larger (relative to the expected data sizes, number of resultants per pixel in this case) than just running the computation directly in C using cython commands despite the fact that the numpy implementations of those numerical operations are much more highly optimized. Steps have also been taken to remove an unnecessary calls out of C to python which also has lead to a modest speed improvement.

Note that there maybe additional speed improvements by explicitly aligning and indicating the contiguous array dimensions in the memory views.

I performed this experiment but found essentially no improvements. In fact it might be slower because our expected input data is not going to be contiguous in the dimension that would result in speed improvements. In order to make this contiguous we would have to copy the input data which is quite expensive.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
setup.py 0.00% <ø> (ø)
src/stcal/ramp_fitting/ols_cas22/__init__.py 100.00% <100.00%> (ø)
tests/test_jump_cas22.py 98.20% <100.00%> (-0.15%) ⬇️

📢 Thoughts on this report? Let us know!

Copy link
Collaborator

@schlafly schlafly 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. A major refactor!

@WilliamJamieson WilliamJamieson marked this pull request as ready for review November 10, 2023 18:08
@WilliamJamieson WilliamJamieson merged commit 41c4ec6 into spacetelescope:main Nov 10, 2023
25 checks passed
@WilliamJamieson WilliamJamieson deleted the bugfix/jump_detect_speed branch November 10, 2023 18:58
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.

3 participants