Bug fix in full numeric pixel likelihood calculation

[gschwefer]

This fixes a bug in the implementation of the full numeric pixel likelihood from de Naurois 2009 , eq 21, in neg_log_likelihood_numeric. The approximate likelihood in neg_log_likelihood_approx is adjusted for consistency, which is important given they are combined in the neg_log_likelihood function. This was not caught in the tests because these also had a bug, which is now fixed too.

I will also implement the fix in the ImPACT branch where the overall pixel likelihood interface changes.

[gschwefer]

What exactly is isort complaining about here?

[Tobychev]

What exactly is isort complaining about here?

Probably you haven't sorted your imports according to the standard scriptures of isort, the easiest fix is just to run isort locally and commit the changes it produces.

Do note that it's not actually isort itself that complains, it just modifies the files as part of the CI, but the files being modified by isort counts as a failure, so you never get to actually hear what it did. Thus the advice to just run it yourself on your own copy.

[Tobychev]

probably mean float

[gschwefer]

Yes

[gschwefer]

Fixed it

[maxnoe]

@gschwefer If you enable the pre-commit hooks, this will automatically run when you commit, see:

https://ctapipe.readthedocs.io/en/latest/getting_started/index.html#developing-a-new-feature-or-code-change

[maxnoe]

This fixes a bug in the implementation of the full numeric pixel likelihood

What is the actual bug here?

I see that you re-introduce terms that are constant under minimization of the likelihood, which is probably the reason why they were left out.

I guess using factorial is pretty slow, here is the optimized implementation of the poisson related functions in numba-stats:
https://github.com/HDembinski/numba-stats/blob/main/src/numba_stats/poisson.py

[gschwefer]

Ok, so these are a couple of different things: The terms in the neg_log_likelihood_numeric that I reintroduce are actually not simple constants to the log likelihood because they are summed over in the n_signal summation and depend on n. Therefore, when the log is taken, they aren't just separate additive terms and therefore make an actual difference. This was not caught in the unit tests because an absolute value was missing in the tests of the relative differences that allowed large negative values to pass.

[gschwefer]

The factorial I'm happy to switch out.

[maxnoe]

The factorial I'm happy to switch out.

Measure before :)

[gschwefer]

The factorial I'm happy to switch out.

Measure before :)

Good hint...the explicit implementation is indeed faster

[maxnoe]

For arrays of k or lambda, numba_stats.poisson.logpmf was a lot faster (factor of 2 or more), maybe worth introducing this additional dependency.

[gschwefer]

For arrays of k or lambda, numba_stats.poisson.logpmf was a lot faster (factor of 2 or more), maybe worth introducing this additional dependency.

Ok, maybe I just don't see it, but my version of numba_stats (1.3.0) doesn't seem to support arrays as input for lambda. And that's what really needed here...

[maxnoe]

Indeed, I only tested arrays for k, I'll open an issue