-
Notifications
You must be signed in to change notification settings - Fork 9
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 likelihood that takes coupling as shape parameters #34
Conversation
Thanks Jingqiang! I can certainly see the use case for this, but without unit tests or an example I can't really tell how this works. If I understand your current implementation correctly, the likelihood will still have the "shape parameters" that mask rate parameters as actual shape parameters. They won't get any values when called due to your argument filtering, but I imagine you still have to pass anchors to initialize them, interpolators will get created on initialization, called during likelihood calls, etc. Did you consider instead writing a wrapper/adapter class that keeps an actual likelihood in a private attribute, rather than something inheriting from |
Hi Jelle, good to hear your comments! Indeed I think it should be cleaner to use a wrapper. I will change it and attach an example notebook when it is done. |
@JelleAalbers @kdund I uploaded an example notebook in this gist. A quick summary is I tested it with multiple gaussian peak signals over a flat background, and the fit results are around the true values as expected. I also added a Currently, couplings as shape parameters are not really constrained by the priors as I guess usually we won't put such constraints on the signal couplings? This can be easily done if you think it's necessary. The con is just it takes some time to build the likelihood but it's ok. |
If we want to add some constraint terms, they can be added as separate logAncillaryLikelihoods ( Line 680 in 7c3d2e8
|
Ah yeah, could be. Or we just add the constraints on the LogLikelihoodVaried. This is not difficult :) |
Hi, I added
|
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.
This looks very nice, thanks for your work Jingqiang! The tests pass for me (after #37).
If you want to be super neat, you can remove the commented def pdf_base_config(self):
code and the empty lines at the end of test_likelihood_reparam.py
. Your history will be preserved when we rebase.
Hey @JelleAalbers, thanks for approving and also the suggestions! Those unnecessary stuff are cleaned up now :D |
Can we merge this PR? :D @JelleAalbers @kdund |
@JelleAalbers @kdund Just let you know that I don't have the write access so I can't merge this PR even if it's approved... can any of you merge the PR? Thanks a lot :) |
In some cases, we have a bunch of signals that depend on shared couplings. For instance, if we have:
Instead of the rate multipliers of signal a, b, and c, we want to set constraints on the coupling
g1
andg2
directly, because in this way:This PR addresses exactly the issue by adding the coupling parameters into the shape parameters. A conversion config
conv_config
is passed to initialize a new likelihood calledUnbinnedLogLikelihoodVaried
so that the likelihood knows how to translate the couplings to rate multipliers for each signal source later. The prior constraint on the coupling parameter itself is also possible.In fact, the new likelihood can handle more than the coupling -- should any shape parameter has a correlation with the signal rate, it can be realized by writing this correlation info in the conversion config.