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

Reproject starfield sample and uncertainty in one fell swoop #323

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

svank
Copy link
Contributor

@svank svank commented Nov 23, 2024

PR summary

Handling uncertainty in the starfield subtraction was just subtracting the actual starfield from the uncertainty through a second call to starfield_model.subtract_from_image. Since it's still the same WCS and transformation, we might as well do it in the same call as for the actual starfield subtraction. That takes things from 10 min to 7m45s.

This does not address the question of what's correct for the uncertainty at this step. It was subtracting the actual starfield, and this PR takes the liberty of changing it to subtract the starfield's uncertainty. Still not correct, but maybe closer.

So the uncertainty comes out different, but the data layer comes out bit-for-bit identical.

@jmbhughes
Copy link
Member

jmbhughes commented Nov 23, 2024

Yay! Does using this require a new version of remove_starfield? If so, we should update our pinned version in the server requirements.

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (dc5615f) to head (34b0aa3).
Report is 103 commits behind head on main.

Files with missing lines Patch % Lines
punchbowl/level3/stellar.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   88.41%   88.43%   +0.02%     
==========================================
  Files          56       56              
  Lines        3746     3745       -1     
==========================================
  Hits         3312     3312              
+ Misses        434      433       -1     

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

@svank
Copy link
Contributor Author

svank commented Nov 23, 2024

I do have pending changes in remove_starfield that will gain another minute or two, and thoughts on next steps for Monday. But this is all in punchbowl!

@jmbhughes jmbhughes self-requested a review November 23, 2024 03:03
@svank
Copy link
Contributor Author

svank commented Nov 23, 2024

I wanted to check the remove_starfield changes a little bit more, but if you just want to crank stuff out this weekend you can pull the main branch. The "blur the input data" stage just directly convolves the data with the right Gaussian, instead of reprojecting it into its own frame twice. As long as there aren't nans or infs in the data, it uses FFT convolution for a big speed gain (on that step).

I don't have timing on that change combined with this, but compared with the "before this PR" state, it also took things from 10 minutes to about 7 min.

@jmbhughes
Copy link
Member

jmbhughes commented Nov 23, 2024

As long as there aren't nans or infs in the data, it uses FFT convolution for a big speed gain (on that step).

We'll want to make sure this is true in punchbowl. Uncertainty can have inf to indicate complete uncertainty, but we can just make the uncertainty a super big number if it speeds things up.

Gotta love FFTs!

@svank
Copy link
Contributor Author

svank commented Nov 23, 2024

We could also swap out uncertainty infs in this stage if it's useful to have them earlier in the pipeline.

@jmbhughes jmbhughes merged commit f0cfdc1 into main Nov 23, 2024
6 of 8 checks passed
@jmbhughes jmbhughes deleted the even-faster-starsub branch November 23, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants