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

implement option in FitterAlgoBase to toggle between nll backends #1028

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pkausw
Copy link

@pkausw pkausw commented Nov 29, 2024

This PR implements an option to forward the switch to toggle nll backends that @guitargeek implemented some time ago to the relevant peace of code. This should work for the MultiDimFit and the FitDiagnostics methods since they internally call the FitterAlgoBase. Other methods can follow as we move along.

This should make it easier to identify breaking points that custom combine classes introduce with pure RooFit code, and would be an important step for the implementation of automatic differentiation in combine.

@anigamova
Copy link
Collaborator

thanks @pkausw! were you planning to use this flag in createNLL call in line https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/src/FitterAlgoBase.cc#L225, right? I guess then we could use it with forceRecreateNLL_ set to true then

@pkausw
Copy link
Author

pkausw commented Dec 4, 2024

Hi @anigamova , sorry for the late reply and thanks for the review! I'm not sure if I understand your question properly. The nllbackend is set as a global flag in the Combine name space here: https://github.com/pkausw/HiggsAnalysis-CombinedLimit/blob/bd3cfa733ba0f351223cd9319cd5065b3679cd75/src/FitterAlgoBase.cc#L111

This global name space is then used in RooSimultaneousOpt to switch between a pure RooFit-based backend or the standard combine backend. So the option is already propagated, and should also work with forceRecreateNLL_, right?

@anigamova
Copy link
Collaborator

@pkausw yes, you are right, I forgot that we already merged this addition to createNLLImpl to the main branch. Thanks!

@@ -85,6 +86,8 @@ FitterAlgoBase::FitterAlgoBase(const char *title) :
("autoBoundsPOIs", boost::program_options::value<std::string>(&autoBoundsPOIs_)->default_value(autoBoundsPOIs_), "Adjust bounds for these POIs if they end up close to the boundary. Can be a list of POIs, or \"*\" to get all")
("autoMaxPOIs", boost::program_options::value<std::string>(&autoMaxPOIs_)->default_value(autoMaxPOIs_), "Adjust maxima for these POIs if they end up close to the boundary. Can be a list of POIs, or \"*\" to get all")
("forceRecreateNLL", "Always recreate NLL when running on multiple toys rather than re-using nll with new dataset")
("nllbackend", boost::program_options::value<std::string>(&nllBackend_)->default_value(nllBackend_), "DEBUG OPTION, DO NOT USE! Set backend to create NLL. Choices: combine (default behavior), cpu, legacy, codegen")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a minor nitpick, but maybe it should be called nllBackend to be consistent with the other options in camelCase?

@guitargeek
Copy link
Contributor

Thanks for figuring out how the options are handled and adding an option for the NLL evaluation backend! To me, this looks all good 👍

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.

3 participants