Change Pick plot of FWHM to match rest of UI report

ejeschke / ginga

The Ginga astronomical FITS file viewer

BSD 3-Clause "New" or "Revised" License

121 stars 77 forks source link

Change Pick plot of FWHM to match rest of UI report #970

Closed ejeschke closed 3 years ago

ejeschke commented 3 years ago

Changes the center point of FWHM calculation in evaluate_peaks from the local peak to the center-of-mass centroid of the local peak, if it can be calculated.

This may result in a slightly more accurate FWHM in the evaluate_peaks pass that can be used to solve the problem outlined in #969

Fix #969

pllim commented 3 years ago

Thanks!

Since @obi-wan76 has a real use case for this, perhaps he is the best person to review.

ejeschke commented 3 years ago

@pllim, if you think this approach has merit, I can push an additional commit to fix Pick to (potentially) solve the problem outlined in #969. We would need some careful testing (mostly with "ill-behaved" objects) to make sure it is working correctly.

pllim commented 3 years ago

I think it is good to have consistent values displayed across plugin and don't see anything wrong with using center of mass. 👍

ejeschke commented 3 years ago

Okay, this PR is amended as follows:

FWHM using local peak as origin (not centroid center), BUT, only using original cutout data

Now plot is the same except for rounding of floats.

ejeschke commented 3 years ago

We can revisit the idea of using the centroid center in a different PR, if desired.

ejeschke commented 3 years ago

@obi-wan76, can you check again with your binary object image using this PR? I think you will find the plot is now consistent with the numbers reported in the rest of the UI. LMK if not.

ejeschke commented 3 years ago

BTW, one side effect of this change is that the graph curves will not align as consistently as before (depending on how close to the edge of the box), although the numbers will. That may alarm some users.

obi-wan76 commented 3 years ago

Screen Shot 2021-08-31 at 11 54 09 AM

Yes, the values are consistent between the figure and Pick Readout. Thanks!

pllim commented 3 years ago

That may alarm some users.

@ejeschke , which one is the lesser of two evils?

@obi-wan76 is happy with this patch, so I am okay with merging as-is unless you still have concerns.

pllim commented 3 years ago

Thanks for the fix! 😄