Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix centroid and gaussian width analysis functions, add distribution-based implementation #1057

Merged
merged 15 commits into from
Jun 16, 2023

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented May 15, 2023

We've had some reports that the uncertainties reported by centroid, gaussian_sigma_width, and gaussian_fwhm are suspect/wrong. This PR will update to using astropy.uncertainty Monte Carlo distributions to estimate the value and uncertainty for these quantities instead of the suspect analytic solutions, and give the option to use an updated analytic solution for centroid that @eteq re-derived. Opening this now to get eyes on my implementation, but I still need to update the gaussian measures.

@eteq
Copy link
Member

eteq commented May 16, 2023

For backstory, a picture of the derivation for the analytic solution is below.

centroid_unc_derivation

I cross-checked this solution and the implementation in this PR in a notebook here:
https://gist.github.com/eteq/37017d89ce3017e387de1e72edb40440

@rosteen, maybe the thing to do is to update this PR to include tests based on that notebook? That is, a test where you generate a fake spectrum with known uncertainty a bunch of times, compute the centroid, and then compare the stddev of those simulations to the uncertanty derived here? As long as you set a random number generator seed for that and a sufficient tolerance it shouldn't lead to mysterious failures due to bad luck of the draw.

@eteq
Copy link
Member

eteq commented May 16, 2023

Oh, and given the growing complexity, maybe we want to change this PR to just the centroid, finish it up, and then do a separate PR for the others?

@rosteen
Copy link
Contributor Author

rosteen commented Jun 6, 2023

Oh, and given the growing complexity, maybe we want to change this PR to just the centroid, finish it up, and then do a separate PR for the others?

I'm going to try to get the gaussian width measures in here too, I'd rather not be inconsistent between those and the centroiding.

A nice side effect of this is that I think I figured out the generalization of the centroiding math to multiple dimensions, so we aren't restricted to 1D or 2D like we were in the previous implementation of centroid.

@rosteen rosteen changed the title Default to using astropy.uncertainty distributions for analysis functions Fix centroid and gaussian width analysis functions, add distribution-based implementation Jun 8, 2023
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Attention: Patch coverage is 94.59459% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.72%. Comparing base (0613ba1) to head (55c497e).
Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
specutils/analysis/location.py 90.32% 3 Missing ⚠️
specutils/analysis/width.py 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1057      +/-   ##
==========================================
+ Coverage   70.35%   70.72%   +0.36%     
==========================================
  Files          64       64              
  Lines        4406     4485      +79     
==========================================
+ Hits         3100     3172      +72     
- Misses       1306     1313       +7     

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

@rosteen rosteen marked this pull request as ready for review June 8, 2023 19:31
@rosteen rosteen requested review from eteq, nmearl and keflavich as code owners June 8, 2023 19:31
@rosteen
Copy link
Contributor Author

rosteen commented Jun 8, 2023

@eteq this is now ready for review. I ended up leaving the default as the analytic solution, since we're fairly certain it's correct now, and leaving the distribution-based solution as an option - I figured this is less disruptive/doesn't change a default that users might not notice. I noticed and fixed a couple other things along the way while adding/fixing tests, for example the sigma width uncertainty was previously just adding sigma.unit to the fractional uncertainty rather than correctly multiplying by sigma itself.

The test failure from the latest run was an unrelated file download error.

@camipacifici
Copy link

From the specviz side: left is old, right is new.
I'd say good job!! :)
Screen Shot 2023-06-13 at 2 34 02 PM

@rosteen rosteen requested review from eteq and removed request for eteq June 15, 2023 14:57
@rosteen rosteen merged commit 148e610 into astropy:main Jun 16, 2023
rosteen added a commit to rosteen/specutils that referenced this pull request Aug 15, 2023
…based implementation (astropy#1057)

* Add option back in to use analytic uncertainty for centroid

* Remove debugging print statements

* Change keyword to be more accurate, updating tests

* Generalize centroid uncertainty calculation to multiple dimensisions

* Update test

* Fix multi-D case for distribution centroid method

* Update gaussian width measures to use distribution-based methods

* Fix codestyle

* Fix mask check and flux_uncert creation

* Parameterizing more tests

* Almost have all tests passing, now raise error if analyalytic=False and no uncertainty

* Cleaning up width after requiring uncertainty for distribution method

* Fix sigma width uncertainty calculations, default to analytic

* Update docstrings

* Add docs
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants