[RF][PyROOT] Pythonize RooFit::DataError to avoid Python 3 syntax errors #9721
Conversation
|
Starting build on |
guitargeek
changed the title
| # 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.
Do we need a test of this new feature e.g. pdf.plotOn(frame, LineColor="kOrange+1") ?
There was a problem hiding this comment.
It's tested in the tutorials
There was a problem hiding this comment.
Yes, but ideally all pythonizations should have their own unit tests here, in _pythonization/test.
There was a problem hiding this comment.
Ok! I have added a new unit test file with this (see the first commit in the PR, which got amended)
| XErrorSize=0, | ||
| ) | ||
| morph_datahist_0p01.plotOn(frame1, Name="morph_dh_cHq3=0.01", LineColor="kGreen", **plot_args) | ||
| morph_datahist_0p25.plotOn(frame1, Name="morph_dh_cHq3=0.25", LineColor="kGreen+1", **plot_args) |
There was a problem hiding this comment.
Ah I see the test for the kGreen+1 is in this other commit.
There was a problem hiding this comment.
Also same comment as for the None pythonization: do we want this in Doxygen too to give it even more visibility (and to have it as reference for users).
There was a problem hiding this comment.
Ok! I added the docs.
|
|
||
|
|
||
| @cpp_signature("DataError(Int_t) ;") | ||
| def DataError(etype): |
There was a problem hiding this comment.
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.
Ok! I added the docs.
There was a problem hiding this comment.
Where is a DataError="SumW2"-like example shown in the docs, sorry?
There was a problem hiding this comment.
It's a few lines below the line you made this comment for
guitargeek
force-pushed
the
dataerror_pyz_1
branch
from
00aa748 to
c923d5a
Compare
|
Starting build on |
|
Hi @etejedor, thanks for the review! I have improved the documentation situation now with and additional commit. After this PR, if you go to the documentation of RooAbsData::plotOn(), you can click on |
Before, `getattr(ROOT, value)` was used to the the color enum value
corresponding to a string, but this commit suggests to use:
```Python
eval("ROOT." + value)
```
The `getattr(ROOT, value)` call had a bit 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`.
In Python 3, you can't do this because `None` is special: ```Python DataError=ROOT.RooAbsArg.None # syntax error! ``` The tutorials used some workarounds that were either obfuscating `None` by using the enum value directly, or used the more verbose `getattr` approach: ```Python DataError=ROOT.RooAbsData.ErrorType(2) DataError=getattr(ROOT.RooAbsData, "None") ``` This commit pythonizes DataError with the usual string-to-enum pythonization pattern already used for `LineColor` or `LineStyle` to avoid this uglieness. One can do now: ```Python DataError="None" DataError=None ```
The pythonizaiton of some RooFit global functions merited their own
documentations, which are added in this commit.
To make these documentations accessible from doxygen, the following
changes were made in addition:
* the `print_roofit_pyz_doctrings.py` script now also considers free
functions
* the global functions are correctly linked to in the documentation of
`RooAbsData::plotOn`
guitargeek
force-pushed
the
dataerror_pyz_1
branch
from
c923d5a to
caa0870
Compare
|
Starting build on |
In Python 3, you can't do this because
Noneis special:The tutorials used some workarounds that were either obfuscating
Noneby using the enum value directly, or used the more verbose
getattrapproach:
This commit pythonizes DataError with the usual string-to-enum
pythonization pattern already used for
LineColororLineStyletoavoid this uglieness. One can do now:
This PR also generalized the string-to-enum pythonization for strings (see the first commit).