-
Notifications
You must be signed in to change notification settings - Fork 14
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
stamp center filter c++ port #407
Conversation
Previous Benchmarks(recorded 10/12/2023)
New Benchmark(recorded 12/19/2023)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few clarification questions
@@ -30,7 +39,7 @@ def setup_coadd_stamp(params): | |||
p = PSF(1.0) | |||
psf_dim = p.get_dim() | |||
psf_rad = p.get_radius() | |||
for i in range(psf_dim): | |||
for i in range(1, psf_dim): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is changing. The calculation below moves the center pixel one off, but shouldn't the convolution with the PSF still cover the full PSF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before the range of the outer for loop would include -1 when i is 0 (due to the one pixel offset), giving an out of bound error. I just changed the range to only include dimensions that fall within the PSF.
@@ -103,7 +103,7 @@ def keep_row(self, row: ResultRow): | |||
return False | |||
|
|||
# Find the peak in the image. | |||
stamp = row.stamp.reshape([self.width, self.width]) | |||
stamp = row.stamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change because stamp is already in the correct shape? Or is there some other reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change.
* porting over first pass * merge * fix bug in array size check * remove commented out code * black changes * fix benchmarking script * code reorganization
ports the code for the stamp center filter into c++.