search

cms-analysis / HiggsAnalysis-CombinedLimit

CMS Higgs Combination toolkit.
https://cms-analysis.github.io/HiggsAnalysis-CombinedLimit/latest
Apache License 2.0
75 stars 384 forks source link

Add flags for SinglePdfGenInfo to avoid using gen spec when changing nuisances per toy. #836

Closed kcormi closed 1 year ago

kcormi commented 1 year ago

We don't want to use a GenSpec object for generating the toys if we want to change the nuisance (i.e. when not doing frequentist toys) values for each toy, since the genSpec object is fixed, and updating the values of the nuisance parameters in the pdf won't change the values the toys are generated with.

For frequentist toys, only the constraint term (i.e. global observable) is changed, which doesn't affect the observed data so using the genSpec is fine.

I've added a flag in the SimPdfGenInfo constructor which enables or disables the use of the spec object when generating events. By default this is allowed, so the default behaviour when this flag isnt passed is unchanged. I think this only needed to be changed when generating the toys from the Combine object itself, as other places where the SimPdfGenInfo object is used seem only to need Asimov Toys, and then there is no problem.

@nucleosynthesis, it would be good to test this with your setup where you spotted the bug, to make sure its actually fixed. I've only run a couple simple sanity checks, not tested it thoroughly.

nucleosynthesis commented 1 year ago

This looks like it works for the example case I was testing. However, this function is also caused when Generating toys from HybridNew - so we would need to also pass the flag if using the option --generateNuisances=1 with HybridNew

kcormi commented 1 year ago

Thanks for point that out -- I'd missed that it was also being called for the HybridNew toys, since they go through a different code path in that case. I've added a flag in the ToyMCSamplerOpt which is stored based on the generateNuisances argument of its constructor, which it then passes along to the SimPdfGenInfo.

I am pretty sure that this ticks all of the boxes, but since the toy generation is slightly more complicated here, I'll spend a bit more time double checking that and running a few tests. But if anyone else spots any issues with this implementation, let me know.

nucleosynthesis commented 1 year ago

Checking with my setup, this now works for HybridNew toys too - thanks!

nucleosynthesis commented 1 year ago

@kcormi - are we ok to merge this now? As far as I can see its good to go

kcormi commented 1 year ago

Sorry for the delayed response, yes, from my side it can be merged.