-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add curve-of-growth-based FWHM estimator #1298
base: main
Are you sure you want to change the base?
Conversation
Hello @eteq! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
hah pep8speaks is very unhappy with me. Good thing I started with a draft 🤖 😠 |
We need to compare this with what is already established in |
I am guessing this is going to be a blocker for both spacetelescope/jdaviz#1048 and spacetelescope/jdaviz#1049 ? Can we even use this without a way to translate between |
@eteq , we discussed this at Indigo tag-up today. I modified your original post with our plan. Please look at it and let us know if you have questions or concerns. |
Honestly I am not convinced we should give the imexam implementation precedence, as the approach in this PR in some ways is intentionally different (e.g., how it treats fractional pixels). I agree a comparison does make sense for sanity-checking and to ensure I'm not making a mistake in something here, but the answer might still be "use this instead of imexam" even if they are different. Rest of the plan looks good to me! Although even if someone else can/wants to take this over I still care a lot about the answer to this algorithm ion |
Oh, and I also think this is already in the right direction - the implementation and API are basically complete modulo any concrete changes. But I would be very happy to get some help making more comprensive tests, as then I could focus just on docs! |
TODO
Original post
This adds a function that basically implements the algorithm I suggested for a project that wants to use photutils: spacetelescope/jdaviz#1048 - I described the algorithm there, but it's basically "put down an aperture, consider "max" to be all of the flux, then find the radius that provides half of that flux to be the half-width-at-half-max.
This isn't the most robust algorithm in the world for FWHMs, but it is probably conceptually the simplest.
This is a working PR, but I should probably document it a bit better and do more comprehensive tests, so I'm opening it for now as a draft. @larrybradley in particular might want to comment if you think I've put this in the right sub-package...
cc @pllim @larrybradley