Add the variables for param NP to the datacard parsing and writing methods #308
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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
paramnuisances to the datacards, attempting to fix it.Also included the
paramvariables to therateParamworkspaces so that the rate parameter formulas with theparamnuisances as arguments could be compiled.