Skip to content
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

Improve WStat implementation #797

Merged
merged 5 commits into from Nov 25, 2016
Merged

Improve WStat implementation #797

merged 5 commits into from Nov 25, 2016

Conversation

@joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 24, 2016

This PR

  • Extends the WStat docs section to explain everything
  • Make the WStat implementation more similar to how Mathieu implemented it because its much more transparent and elegant (and gives you better access to intermediate results, so you can also be happy @cdeil )
@joleroi joleroi added this to the 0.6 milestone Nov 24, 2016
@joleroi joleroi self-assigned this Nov 24, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 24, 2016

There's a few typos on the docs page, e.g. formular, unphyisical, distince - PyCharm has a spell-checker. :-)

Copy link
Member

@cdeil cdeil left a comment

I've left some inline comments.
Will have a more detailed look when it's in master.

Special cases
^^^^^^^^^^^^^

The above formular is obviously undefined if :math:`n_{\mathrm{on}}` or :math:`n_{\mathrm{off}}`

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

"obviously undefined" -> just say why: log of zero or whatever the issue is.

This comment has been minimized.

@joleroi

joleroi Nov 25, 2016
Author Contributor

You are right, usually it means that the author didn't exactly know why

Otherwise, two cases are distinguished.
Obviously, :math:`\mu_{\mathrm{bkg}}` can become negative which is unphyisical.

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

Again, I don't like "obviously" in math texts. Either remove "obviously", or if it's quick to say where, it becomes negative, do so.

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

unphyisical -> unphysical

r"""W statistic, for Poisson data with Poisson background.
For a definition of WStat see :ref:`wstat`.
For a definition of WStat see :ref:`wstat`. If ``mu_bkg`` is not provided
it will be calculated according to the profile likelihood formular.

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

formular -> formula (everywhere)

term3 = - n_off * np.log(mu_bkg)
term2_ = - n_on * np.log(mu_sig + alpha * mu_bkg)

zero_val = np.zeros(len(n_on))

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

Is this needed. Can't you just use 0 below and Numpy broadcasting will take care of this because always n_on and n_off have the same length?

This comment has been minimized.

@joleroi

joleroi Nov 25, 2016
Author Contributor

You are right, I didn't know one could do that 👍


special_case[idx] = first_term + second_term
return special_case
zeros = np.zeros(len(n_on))

This comment has been minimized.

@cdeil

cdeil Nov 24, 2016
Member

Again, is this really needed?
Don't you get the same result if you just put scalar 0 below?

@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Nov 25, 2016

PyCharm has a spell-checker. :-)

Vim too 😉

@joleroi joleroi force-pushed the joleroi:last_wstat_branch branch from ae845c4 to 2e84967 Nov 25, 2016
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Nov 25, 2016

I am merging this, let's continue the discussion on master (but IMO the docs/implementation have converged)

@joleroi joleroi merged commit 7ba231e into gammapy:master Nov 25, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joleroi joleroi deleted the joleroi:last_wstat_branch branch Nov 25, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 25, 2016

From: http://docs.gammapy.org/en/latest/stats/fit_statistics.html#poisson-data-with-background-measurement

Intuitively, this log-likelihood ratio should asymptotically behave like a chi-square with m-n degrees of freedom, where m is the number of measurements and n the number of model parameters.

Maybe look at this distribution and compare to the expected distribution some day for one or two simple cases (e.g. PL spectral model with 10 on-off-bins or something like that)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants