root icon indicating copy to clipboard operation
root copied to clipboard

[RF] Implement style and color command argument Pythonizations in C++

Open guitargeek opened this issue 1 year ago • 1 comments

This is done to reduce the feature divergence between PyROOT and C++ ROOT. Also improves code performance and robustness.

guitargeek avatar Oct 17 '24 15:10 guitargeek

Test Results

    18 files      18 suites   4d 2h 28m 25s ⏱️  2 699 tests  2 698 ✅ 0 💤 1 ❌ 46 103 runs  46 102 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit a62c2f3e.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Oct 17 '24 19:10 github-actions[bot]

Thanks a lot for the review and the questions!

* Are we sure it's not possible to avoid the extra call to the interpreter? That is not done in the current status (there is the `eval` but I don't think it calls directly into cling every time)

That's true, but probably eval is even worse. It's the gInterpreter of Python, which is "many orders of magnitude slower for smaller expressions/objects than plain ol’ Python": https://pandas.pydata.org/pandas-docs/version/0.20/enhancingperf.html#expression-evaluation-via-eval-experimental

Also, it's more flexible. Since in Python, you have to do ROOT.kRed, while gInterpreter can just take kRed, there was this hack of adding "ROOT." to the string passed to eval, which reduced flexibility and would result in unexpected errors for example when the user does "1+kRed" instead of "kRed+1".

* What happens after this change to keyword arguments? Is this tested somewhere? For instance, can I call the same Python function with a random order of the keyword arguments even after these changes?

It is tested in all RooFit tutorials that do plots, and it works just fine. And yes, the keyword argument pythonization still works.

guitargeek avatar Oct 29 '24 10:10 guitargeek