-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conditional Parameters #186
Conversation
Pull Request Test Coverage Report for Build 10278961342Details
💛 - Coveralls |
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.
Thanks Robert for this elegant solution. I didn't run anything but just read through the codes, since you already see it is working and I think the new test is checking the core feature. I think the absence of support for "on-the-fly" doesn't hurt, since we don't expect overriding mass for example in massive production. In notebook, a mistake here won't cost much so I would say we are good to merge now.
This is a copy of #181 (I accidentally merged it and had to revert the main....)
This PR introduces conditional parameters.
What's the problem?
In some cases, we want parameters to take different values depending on the value of other parameters.
One example is the WIMP-mass-dependent signal efficiency uncertainty. Here, we typically have larger uncertainties for lower masses. With conditional parameters, we can now simply add the following to our parameter definition in the config file:
Depending on the value of
wimp_mass
this will result in a different value of thesignal_efficiency
uncertainty.The ConditionalParameter class
Here's an example how the ConditionalParameter class works:
Caveat
So far, this doesn't work "on-the-fly" i.e. if I have a model instance and just change the
wimp_mass
value it won't correctly update the treatment of the signal efficiency since the likelihood is already initialized with the constraint terms. However, the structure introduced in this PR should allow introducing this feature in the future.What I tested so far
I tested this in a notebook for a few different WIMP masses (this won't work with the config in alea since we don't have any other WIMP mass templates yet, but you can ask me for an example notebook using actual templates). So far, everything looked good and behaved as expected but heavier testing with large toyMCs will follow.
Please have a look, play around with the new class, and let me know if something unexpected happens. Also, if you have any suggestions on how to implement this better, please let me know! In the meantime, I'll write some unittests and do some first checks with toyMCs.