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

Normalize patches by the star-center value rather than the patch maximum #60

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

svank
Copy link
Contributor

@svank svank commented Aug 25, 2023

Btw if you're gearing up for another release, there was one last local change I never pushed. When the cutouts of each star are being collected, each one gets normalized. The was done by dividing each patch by its maximum value. In cases where there's some contaminant in the patch that's brighter than the star, the normalized star is in the range [0, small] instead of [0, 1] and so doesn't contribute well to the final PSF model. I changed it to use the patch-center value (i.e. the star's center and presumably peak value) rather than the patch's maximum value, to help ensure that the stars are all normalized the same. Any bright contaminants will be normalized to values > 1, but since they're hopefully rare, that shouldn't really affect the median/percentile/whatever.

If the patch sizes are small, hopefully these contaminants are pretty rare and this is a low-impact change.

What do you think?

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (93ab305) 94.57% compared to head (4d58b0b) 94.57%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #60   +/-   ##
=======================================
  Coverage   94.57%   94.57%           
=======================================
  Files          11       11           
  Lines         996      996           
=======================================
  Hits          942      942           
  Misses         54       54           
Files Changed Coverage Δ
regularizepsf/fitter.py 90.00% <100.00%> (ø)
tests/test_fitter.py 100.00% <100.00%> (ø)

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

@jmbhughes jmbhughes self-requested a review August 25, 2023 19:47
@jmbhughes jmbhughes added the enhancement New feature or request label Aug 25, 2023
@jmbhughes jmbhughes changed the base branch from main to develop August 25, 2023 19:52
@jmbhughes
Copy link
Member

This is a pretty minor so I probably can just include it. It seems like a good idea. It speeds things up (likely very minor since the patches are small) by also not computing a max of a patch but just accessing one value. I'll think about any unforeseen side effects and I'll merge early next week probably.

I changed it to merge into develop. Once we have all the improvements assembled, we'll merge into main and roll a release.

@jmbhughes jmbhughes merged commit a0dffba into punch-mission:develop Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants