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

Add curve-of-growth-based FWHM estimator #1298

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

eteq
Copy link
Member

@eteq eteq commented Feb 18, 2022

TODO

  • Name a Jdaviz dev to take over this PR.
  • Compare this to what is already in imexam. Which one should we use?
  • Finalize API.
  • Implement chosen algorithm.
  • Add, update, and fix tests.
  • Add and update documentation.
  • Update change log, if necessary.

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

@pep8speaks
Copy link

Hello @eteq! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 14:33: E128 continuation line under-indented for visual indent
Line 20:1: E303 too many blank lines (3)
Line 98:101: E501 line too long (118 > 100 characters)
Line 121:101: E501 line too long (138 > 100 characters)
Line 142:13: E265 block comment should start with '# '
Line 142:101: E501 line too long (104 > 100 characters)
Line 153:88: E262 inline comment should start with '# '
Line 153:101: E501 line too long (144 > 100 characters)
Line 160:101: E501 line too long (101 > 100 characters)
Line 161:66: E225 missing whitespace around operator
Line 162:34: E127 continuation line over-indented for visual indent
Line 182:101: E501 line too long (112 > 100 characters)
Line 184:42: W292 no newline at end of file

Line 1:1: F401 'curses.COLOR_GREEN' imported but unused
Line 13:1: E302 expected 2 blank lines, found 1
Line 20:32: E261 at least two spaces before inline comment
Line 20:32: E262 inline comment should start with '# '
Line 21:26: E231 missing whitespace after ','
Line 21:28: E231 missing whitespace after ','
Line 21:30: E231 missing whitespace after ','
Line 26:20: E261 at least two spaces before inline comment

@eteq
Copy link
Member Author

eteq commented Feb 21, 2022

hah pep8speaks is very unhappy with me. Good thing I started with a draft 🤖 😠

@pllim
Copy link
Member

pllim commented Feb 21, 2022

We need to compare this with what is already established in imexam, see https://github.com/spacetelescope/imexam/blob/b3975733e0ef459fc7d137acec9bf4aa2865d6c5/imexam/imexamine.py#L1266 . It also uses photutils for photometry.

@pllim
Copy link
Member

pllim commented Feb 21, 2022

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 regions region and photutils region first?

@pllim
Copy link
Member

pllim commented Feb 24, 2022

@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.

@eteq
Copy link
Member Author

eteq commented Mar 8, 2022

We need to compare this with what is already established in imexam,

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 photutils regardless of how it gets used in jdaviz.

@eteq
Copy link
Member Author

eteq commented Mar 8, 2022

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!

@larrybradley larrybradley removed this from the 1.4.0 milestone Mar 25, 2022
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.

None yet

4 participants