Skip to content

Conversation

@guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Mar 13, 2022

In case of named arguments, the RooFormula will replace the argument
names with x[0] to x[n]. There are two things that can go wrong if
RooFormula is not implemented right. First, if there is a variable named
"x" it should only be substituted if the matching substring is not
followed by "[", to not replace existing x[i]. Second, variables with
integer names like "0" should only be substituted if the match is not
followed by a "]", again to avoid replacing x[i]. This test checks that
these cases are handled correctly.

The second case was so far not dealt with correctly, but with this
commit it is. A corresponding unit test was also implemented.

The preprocessor commands in RooFormula were also reorganized
slightly, such that one can test the TPRegexp backend simply by
commenting out the define ROOFORMULA_HAVE_STD_REGEX.

This pull request fixes an issue reported in the forum:
https://root-forum.cern.ch/t/a-strange-bug-in-rf708-bphysics-c/49152/2

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

In case of named arguments, the RooFormula will replace the argument
names with` x[0]` to `x[n]`. There are two things that can go wrong if
RooFormula is not implemented right. First, if there is a variable named
"x" it should only be substituted if the matching substring is not
followed by "[", to not replace existing x[i]. Second, variables with
integer names like "0" should only be substituted if the match is not
followed by a "]", again to avoid replacing x[i]. This test checks that
these cases are handled correctly.

The second case was so far not dealt with correctly, but with this
commit it is. A corresponding unit test was also implemented.

The preprocessor commands in `RooFormula` were also reorganized
slightly, such that one can test the `TPRegexp` backend simply by
commenting out the `define ROOFORMULA_HAVE_STD_REGEX`.
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you the fix.
I have one general comment, do we really need to support variables which have integer names ?
This can be confusing and in my opinion people should avoid doing this, and if RooFit does internally we should set a name such as "v " + integerValue.

@guitargeek
Copy link
Contributor Author

Hi @lmoneta, I agree with you that these number literal names are confusing, but I'm afraid we can't get rid of this without risking to break user code.

It was always the case that the RooConstVar created with RooFit::RooConst have their value directly as a name. If we would change that now, it could have unexpected consequences for user code.

@guitargeek guitargeek merged commit 776f900 into root-project:master Mar 14, 2022
@guitargeek guitargeek deleted the RooFormula_2 branch March 14, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants