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

explicit unconditioned bf and -2llhr instead of just -llhr #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

plt109
Copy link
Collaborator

@plt109 plt109 commented May 19, 2023

Changes and motivation
The changes in this pull request and their associated motivations are:

  1. Explicitly computes the loglikelihood value at the unconditioned bestfit instead of just taking the max loglikelihood available in the scan range of the target parameter the user requested. In this way, even if the unconditioned bestfit is not within the scan range set by the user, the loglikelihood ratio will not be wrong.
  2. Plots the -2 log likelihood ratio instead of the -log likelihood ratio so that it is easier to see if the resulting limit(s) is/are cutting at the anticipated values. Wilks' theorem said that under certain asymptotic and regularity conditions, the -2log likelihood ratio follows the chi2 distribution of the appropriate number of degrees of freedom. Perhaps it is just convention but usually the critical value is for the -2 log likelihood ratio test statistic instead of the - log likelihood ratio test statistic and it would be easier for the user to overlay the profiled loglikelihood ratio and the critical value as a function of the target parameter if the y-axis is -2 loglikelihood ratio instead.
    m2llhr

Testing
The unit test that involves plot_likelihood_ratio was commented out because 'For now just test that it doesn't crash -- image comparison tests are tricky...'. To ensure that there are no breaking changes, the plot in the tutorial notebook was reproduced (but with -2 log likelihood ratio instead):
image
The first plot in this PR was also plotted using plot_likelihood_ratio from this commit.

Copy link
Owner

@JelleAalbers JelleAalbers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pueh, this looks good! It only changes a debugging/plotting function, which I don't think (m)any people depend on. You are better placed than I am to judge what is in the XENON code these days of course :-).

Could you commit your changes to the notebook (including the new figures)? Just so the little bit of documentation we have does not go out of date ;-)

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

Successfully merging this pull request may close these issues.

2 participants