Skip to content
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 flags for SinglePdfGenInfo to avoid using gen spec when changing nuisances per toy. #836

Merged
merged 4 commits into from
May 9, 2023

Conversation

kcormi
Copy link
Collaborator

@kcormi kcormi commented Apr 25, 2023

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.

We don't want to use a GenSpec object for generating the toys if
we want to change the nuisance values for each toy, since the genSpec
object is fixed.

For frequentist toys, only the constraint term (i.e. global observable)
is changed, which doesn't effect the observed data so using the genSpec is fine.
@nucleosynthesis
Copy link
Contributor

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
Copy link
Collaborator Author

kcormi commented Apr 28, 2023

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
Copy link
Contributor

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

@nucleosynthesis
Copy link
Contributor

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

@kcormi
Copy link
Collaborator Author

kcormi commented May 9, 2023

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

@nucleosynthesis nucleosynthesis merged commit ad80b60 into cms-analysis:main May 9, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants