Skip to content

Conversation

@guitargeek
Copy link
Collaborator

Draft PR for @PetroZarytskyi to pick up the work.

@guitargeek guitargeek marked this pull request as draft November 4, 2025 14:22
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@guitargeek guitargeek force-pushed the ad_dev branch 2 times, most recently from ffa878b to ed48772 Compare November 5, 2025 11:52
@ajgilbert
Copy link
Collaborator

Hi, maybe just to understand better - is the plan to develop a proper AD implementation for these classes? I had in mind to restructure the code a little anyhow. I'm also not sure how easily the interaction with the analytic minimisation will work, since I think we need to differentiate the likelihood profiled over the Barlow-Beeston parameters.

@guitargeek
Copy link
Collaborator Author

Hi @ajgilbert, thanks for asking!

Yes. The plan is to implement the translation to simple C++ code for these classes so that we can use RooFit "codegen" backend that also allows you go get analytical gradients with AD.

Some pointers to explain this technology:

We focused a lot on implementing automatic tests in the last days, so we can more confidently refactor Combine to make this work for more Combine classes, including the analytical Barlow Beeston.

As you see from this draft PR, adding codegen support for more classes is separate from the standard evaluation path, so this development would now clash much with your developments. If you intend to improve these CMSHist classes, we would greatly appreciate it because it will for sure make it easier to understand what these classes do if the implementation is improved!

Yes, we know that we need to differentiate through the analytical minimization, and we also have a plan there. From the discussions at the CAT hackathon last week, I understand that one requirement would be that analytical minimization can be easily switched on and off for each parameter after creating the likelihood, so that one can do also Minos scans for individual parameters while analytically minimizing the others.

So here is the plan. Right now, RooFit codegen translates the likelihood to differentiable C++ code that looks like this:

double nll(double *params, const double *obs, const double *xlArr)
{
    // params: the RooRealVar and RooCategory parameters
    // obs: info from the dataset (point data, bin centers, weights and weights squared)
    // xlArr: auxiliary constants (template histograms, index mappings, etc.)

    // C++ code for likelihood, generated by RooFit "codegen"
}

To do analytical minimization on top of this, we would just-in-time compile two more functions. One that does the analytical minimization, and one NLL wrapper that uses it:

double analytical_minim(double *params, const double *obs, const double *xlArr)
{
    // params are mutated here according to analytical Barlow Beeston
    // analytical minimization can be switched on and off for dindividual params,
    // with flags that are encoded in xlArr or RooCategory "params"
}

double nll_with_analytical_bb(double *params, const double *obs, const double *xlArr, bool doMinim)
{
    double params_minimized[nParams]{}; // nParams is hardcoded in code generation
    if (minim) { // flags like this will be taken from xlArr or RooCategory values in "params"
       params_minimized[1] = analytical_minim(params, obs, xlArr);
    } // else params_minimized will just be params copied.
    return nll(params_minimized /*not params anymore*/, obs, xlArr);
}

I would like to support this concept of analytical minimization of some parameter also generally in RooFit, so we avoid adding more specific code to Combines CachingNLL classes.

So there are some steps that we need to take:

  • Support this pattern of re-assigning parameters in Clad, the AD tool (progress tracked in Regression in Clad 2.2 when assigning to arrays vgvassilev/clad#1637)
  • Refactor the analytical minimization in Combine to be happening at the beginning of the likelihood function (WIP Do analytical Barlow Beeston before likelihood evaluation #1137), so that it doesn't happen in "random" places in the computation graph that require hacking the "dirty flag" propagation
  • Further refactor the analytical minimization to not happen in the CachingNLL, but in a separate class that sits on top of the likelihood (RooAnalyticalMinimizer?). Then the computation graph more easily maps to the separate generated C++ functions that I was sketching out before. This class would traverse all model RooAbsArg components and collect analytical minimization hooks via a new virtual RooAbsArg interface that is yet to be added to RooFit
  • This RooAnalyticalMinimizer class would also go into upstream RooFit, where we extensively test it and also implement code generation as lined out before
  • Profit from this in CMS Combine, differentiation through the analytical Barlow Beeston

Finally we do some benchmarks with profile likelihood scans, because that's where one likelihood function is re-used in many minimization passes, and we expect AD to shine there (for individual minimizations, the overhead of compiling the gradient might not be worth it, but who knows how much we can still optimize it).

Any feedback on this plan or further ideas are warmly welcome!

FYI: @PetroZarytskyi @vgvassilev

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.

2 participants