-
Notifications
You must be signed in to change notification settings - Fork 194
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 different options to compute stat_array on FluxPointsDatasets #5135
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5135 +/- ##
==========================================
+ Coverage 94.25% 94.46% +0.20%
==========================================
Files 234 235 +1
Lines 35226 35493 +267
==========================================
+ Hits 33204 33528 +324
+ Misses 2022 1965 -57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QRemy ! The profile
is an elegant solution for considering upper limits during the fit!
I don't understand the implementation of the distrib
option. Maybe you can add some information in gammapy.stat
docs page?
Otherwise, only minor comments inline
Thanks @AtreyeeS I implemented most of the comments.
Assuming gaussian errors the likelihood is given by the probability density function of the normal distribution.
|
Thanks @QRemy, the statistical approach to handle the ULs looks reasonable to me. I was wonderign whether we can maybe find a more descriptive name. I think even "chi2-marginalize-ul" would be better. We should definitely give the paper reference in addition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @QRemy ! Elegant indeed.
Maybe you can add a little info the documentation (what you wrote in the comment, and the paper reference).
And add stat_type
option in the example here
dataset_fp.models = model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distrib
option is often returning a nan
for some bins, leading to the total stat becoming nan
and a failure of the minimisation. Any idea why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QRemy ! I have left some comments inline.
My main comment is that you could compute most of the necessary arrays in advance. I suppose that for large SEDs this might be much more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QRemy.. I have some questions for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QRemy . All good for me;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QRemy . This looks good to me.
I have left a simple comment regarding not providing the dictionary of stat functions but simply their keys.
Otherwise, this would be much cleaner with a stat functions being defined as external objects. This can be a further PR though.
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Co-authored-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Co-authored-by: Bruno Khélifi <khelifi@in2p3.fr>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Add different options to compute stat_array on FluxPointsDatasets :
assumes that flux points correspond to asymmetric gaussians and upper limits complemantary error functions.
Default is
chi2
, in that case upper limits are ignored and the mean of asymetrics error is used.The
distrib
case provides an approximation if theprofile
is not availablewhich allows to take into accounts upper limits and asymetrics errors.
Updated /tutorials/analysis-1d/spectral_analysis.py to present the different cases:
Without ul the results are the same: