Cstat fix #85
Cstat fix #85
Conversation
Codecov ReportBase: 54.63% // Head: 54.79% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 54.63% 54.79% +0.15%
==========================================
Files 19 19
Lines 3115 3119 +4
==========================================
+ Hits 1702 1709 +7
+ Misses 1413 1410 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
samaloney
force-pushed
the
kris-cstat0s
branch
from
f1cfd06
to
a185fa0
Compare
There was a problem hiding this comment.
Nice fix, I think it needs a test e.g. calling cstat_loglikelihood function with some data with zeros and making sure it behaves as intended.
Also none of these show up in the doc build which makes it hard to review the changes to the docs would be good to get that sorted maybe not in this PR. I've open a different PR to add the docs #94 you can see a built version here https://sunxspex--94.org.readthedocs.build/en/94/
Co-authored-by: Shane Maloney <shane.maloney@dias.ie>
samaloney
force-pushed
the
kris-cstat0s
branch
from
dcb610d
to
e757c8b
Compare
| if len(observed_counts[_zero_data]) > 0: | ||
| likelihoods[_zero_data] = -model_counts[_zero_data] |
There was a problem hiding this comment.
Is the if statement necessary? _zero_data is a boolean mask so should always work. Unless it a perforce thing but might be faster too?
* Fix bug in cstat calculation with 0 count values * Added test for c-stat and found/fixed fault in my previous fix while doing so. * Squeeze away extra array dimensions not needed in cstat.
The C-stat likelihood doesn't handle bins of 0 observed counts correctly. In this case, revert to the Poissonian log-likelihood. This means at Data=0, ln(likelihood)=-model.