-
Notifications
You must be signed in to change notification settings - Fork 182
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 the variables for param NP to the datacard parsing and writing methods #308
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.
Hi @anigamova thanks a lot for doing this, sorry the code is such a mess. I agree that the changes will have the right effect, though I have a slight concern that there could be side effects in other parts of CH, to do with evaluating the model. As we don't have any unit tests or CI (also my fault), it's hard to say for sure. I put a couple of suggestions in the code for a slightly different way to solve it, that I think will be safe against any side effects. See if you think it makes sense.
@@ -318,6 +314,10 @@ int CombineHarvester::ParseDatacard(std::string const& filename, | |||
param->set_range_u(boost::lexical_cast<double>(tokens[2])); | |||
} | |||
} | |||
auto sys = std::make_shared<Systematic>(); |
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.
I have a worry this could cause problems in other bits of code, e.g. PostFitShapes, where it tries to evaluate all the Systematic objects, and might not know what to do with a param
type. It might also be fine, I'm not sure. I can see another way to fix this while avoiding creating a Systematic
entry. It would be to add a new bool data member to the Parameter
class that flags parameters that come from param
lines in the datacard. Can make false by default, and only set it to true here.
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.
Hi @ajgilbert, thanks a lot for the feedback,
I think in the PostFitFromWorkspace method the datacards are setup from the workspace, and the datacard is parsed only for the re-binning, and I checked that it works. In principle I'm fine with reverting it back and avoiding definition of Systematic
entry, but I also think that it would be good to be able to setup the param
nuisances from CH datacard creation workflow, so that you can define formula based rate parameters. I just pushed a couple of commits that do that. Let me know what you think
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.
Ok yes, I see the point, indeed it's useful if these can be created directly. Do you have any other commits to add? If not we can merge.
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.
I think I do not have anything to add at this point, thanks!
@@ -1016,6 +1019,21 @@ void CombineHarvester::WriteDatacard(std::string const& name, | |||
} | |||
} | |||
|
|||
for (auto par : params_) { |
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.
if following changes above, could remove this part, and instead modify the block that writes param
lines around L1241, to replace all_dependents_pars.count(p->name())
requirement by all_dependents_pars.count(p->name()) OR [new bool flag is set]
.
At the moment CH is not able to parse and write the
param
nuisances to the datacards, attempting to fix it.Also included the
param
variables to therateParam
workspaces so that the rate parameter formulas with theparam
nuisances as arguments could be compiled.