-
Notifications
You must be signed in to change notification settings - Fork 393
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 regex for floatNuisances #851
Conversation
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! This is a nice addition.
Since what is being done here is essentially a copy of what is already done for freezing the nuisance parameters (https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/src/Combine.cc#L607) it would be good not to repeat the code, and perhaps instead wrap this functionality in a function, which can then be called for both the floating and freezing case, just with a different set of arguments.
That would be nice for readability and making future changes less error prone.
I think it would also be good to add that this is done to the option description, as e.g. done for --trackParameters
here: https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/src/Combine.cc#L161
I'm not sure if it's really necessary, but perhaps we should also add a brief clause to the documentation:( https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/docs/part3/runningthetool.md?plain=1#L58) further stating that the same regex for freezeParameters
applies for floatParameters
.
Then I'd be happy to merge this in. Thanks!
@kcormi Added, thanks! Let me know if there are any issues. |
Thanks @rkansal47, sorry for the slow reaction. I've decided to move the documentation so that it only gets updated with the next version, so that the documentation is for the officially recommended version. Thats in #866 and will be merged when we update the version. |
Solves #850.