-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[RF][PyROOT] Pythonize RooFit::DataError to avoid Python 3 syntax errors #9721
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,7 +70,11 @@ def _string_to_root_attribute(value, lookup_map): | |
return getattr(ROOT, lookup_map[value]) | ||
else: | ||
try: | ||
return getattr(ROOT, value) | ||
# Here getattr(ROOT, value) would have less overhead, but it's | ||
# also less convenient. With `eval`, the string that is passed | ||
# by the user can also contain postfix operations on the enum | ||
# value, which is often used for columns, e.g. `kGreen+1`. | ||
return eval("ROOT." + value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a test of this new feature e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's tested in the tutorials There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but ideally all pythonizations should have their own unit tests here, in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok! I have added a new unit test file with this (see the first commit in the PR, which got amended) |
||
except: | ||
raise ValueError( | ||
"Unsupported value passed. The value either has to be the name of an attribute of the ROOT module, or match with one of the following values that get translated to ROOT attributes: {}".format( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import unittest | ||
|
||
import ROOT | ||
|
||
|
||
class TestRooGlobalFunc(unittest.TestCase): | ||
""" | ||
Test for RooGlobalFunc pythonizations. | ||
""" | ||
|
||
def test_color_codes(self): | ||
"""Test that the color code pythonizations in the functions like | ||
RooFit.LineColor are working as they should. | ||
""" | ||
|
||
def code(color): | ||
"""Get the color code that will be obtained by a given argument | ||
passed to RooFit.LineColor. | ||
""" | ||
return ROOT.RooFit.LineColor(color).getInt(0) | ||
|
||
# Check that general string to enum pythonization works | ||
self.assertEqual(code(ROOT.kRed), code("kRed")) | ||
|
||
# Check that matplotlib-style color strings work | ||
self.assertEqual(code(ROOT.kRed), code("r")) | ||
|
||
# Check that postfix operations applied to ROOT color codes work | ||
self.assertEqual(code(ROOT.kRed+1), code("kRed+1")) | ||
|
||
|
||
if __name__ == "__main__": | ||
unittest.main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! It would be good to have this new addition in the Doxygen docs of this file, right? Besides modifying the tutorials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I added the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is a
DataError="SumW2"
-like example shown in the docs, sorry?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a few lines below the line you made this comment for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw it now, thanks!