Pick: EE calculation

[pllim]

Fix #933

TODO

• [x] Do we really need encircled energy? If so, how to calculate without photutils? (Used code from poppy.)
• [x] Do we really need a separate background calculation? iqcalc already has a ~sky level~ median. (Use what iqcalc has for now.)
• [x] How do we improve ensquared energy calculation? Right now, it isn't very robust. (I think it is better now?)
• [x] Add a input box for radius to calculate EE from. ~Right now, it is hardcoded to half of FWHM.~
  • [x] Do we really have two different radii here? One to define the box for the entire sum, and one to define where to measure the value that goes out to the results table. (Yes)
• [x] Put that radius in the plot too.
• [x] Have people test this. Does this break the calc_quick mode etc?
• [x] Update doc: ~API~ (not published), user-facing, screenshots
• [x] Add tests for iqcalc

Screenshots

(The plot looks a little better than this screenshot now, but you get the idea.)

[pllim]

@shanosborne and @obi-wan76 , is this more or less what you had in mind? Do you want to take this feature branch for a spin?

[obi-wan76]

@pllim It looks great, thanks! Yes, we'll give it a try and let you know.

[ejeschke]

Nice work, @pllim! I confirmed that it works correctly with "Show candidates" and also "Quick mode" still works as expected. Plot looks good.

[ejeschke]

I don't necessarily suggest it for this PR, but one thing you can do later is provide settings for screening candidates. For example, you could have min/max EE to choose the preferred candidate (just like there is currently a min/max FWHM). Better to wait to see if that is something that is needed though...

[pllim]

min/max EE to choose the preferred candidate

If I remember correctly, EE for a PSF is pretty much fixed for a given instrument/filter combo. So, I don't see the point of this feature if that is true. There could be a variation across X/Y of the detector or maybe even change with time, but those are calibrated out on the instrumentation side, no?

I'll wait for any more changes

Yes, that sounds good. I am awaiting user feedback before I wrap up this PR by adding tests and docs.

[obi-wan76]

Hi @pllim, this new addition looks great, thanks so much for such a great effort and contribution.

I've been playing with the tool in a semi-crowded star field and I am getting some inconsistent results. I attached a couple of screenshots of two PSF extractions: for a bright PSF and a relatively fainter PSF. For the case of the bright PSF I get the EE values and the the EE curve, but when selecting the relatively fainter (but good SNR) PSF I don't get any value, EE =0. This is the same behavior when I select several PSF at the same time, with some showing EE values while other don't. Am I missing some selection cut off? I've been playing with most of the parameters in "Setting" with the same results.

Thanks!

[pllim]

@obi-wan76 , zeroes mean something went wrong. The negative values probably means background is over-subtracted. Please DM me the locations of these files and the expected values at a given pixel radius. Thanks!

I tried to use as much things from existing code as I could, so the background is what Ginga already calculates, and the total flux (where EE=1) is estimated from a half-width/radius of FWHM (I don't have any good reason for this). Perhaps not all existing calculation is compatible with EE calculations.

[ejeschke]

Am I missing some selection cut off? I've been playing with most of the parameters in "Setting" with the same results.

@obi-wan76, it looks like (from the first screenshot and your report) that the candidates are being selected, since you are getting FWHM readings and entries in the table (albeit with a EE value of 0). So I don't think it is an issue with selection...

[pllim]

Looks like using FWHM to estimate where EE should be 1 is flawed. I might need to add another text box to let you specify that, as you originally requested.

[pllim]

@obi-wan76 -- I think I fixed the problems. Now you can specify the radius where you expect EE fraction to be 1. And the background is now taken from the median, which is consistent with FWHM fitting. I am not sure what the skylevel is for. Can you please grab the updated code from the branch and try again? Thanks!

[pllim]

Not sure why older_deps job failed with this error. Maybe transient; let's wait and see.

E: Failed to fetch https://packages.microsoft.com/ubuntu/16.04/prod/dists/xenial/main/binary-amd64/Packages
    Writing more data than expected (1113755 > 1111551)
E: Some index files failed to download. They have been ignored, or old ones used instead.

[pllim]

Sorry, @obi-wan76 , you might have to pull the new changes I added today and test this (again). I am stopping at 37e0010 until I hear back, I promise!

[pllim]

@ejeschke , can you please share with me the data file for screenshots at https://ginga.readthedocs.io/en/latest/manual/plugins_local/pick.html ? Would be nice if you could upload it to https://stsci.app.box.com/folder/3381606676 . Thanks!

[ejeschke]

@pllim, see file SUPA01118766.fits

[pllim]

@ejeschke , @obi-wan76 , @shanosborne -- I think this is ready for a final review. Rendered doc is at https://ginga--936.org.readthedocs.build/en/936/manual/plugins_local/pick.html (compared with the current dev version at https://ginga.readthedocs.io/en/latest/manual/plugins_local/pick.html) . Please approve if you are okay with merging this. Thanks!

[obi-wan76]

@pllim I just tested the latest version and it work as intended. I see the addition of the EE_r into the table, looks good. I am ok with merging this.

[pllim]

Thanks, @obi-wan76 ! @ejeschke , shall we merge? 😸

[ejeschke]

@pllim, FFTM.