Fix doxygen generation with Python 3 #3434
Conversation
|
Can one of the admins verify this patch? |
|
I see you also changed some a the python tutorials. I will ask our python expert to review. |
|
Thank you @ellert , were the changes of the tutorials intended to be part of this doxygen-related PR? If so, I would perhaps place them in a separate commit. I will comment on the changes. |
|
|
||
| # A simple helper function to fill a test tree: this makes the example | ||
| # stand-alone. | ||
| def fill_tree(treeName, fileName): | ||
| d = RDataFrame(25000) | ||
| d = ROOT.ROOT.RDataFrame(25000) |
There was a problem hiding this comment.
What is the reason to re-add this duplication? Is this not working properly in Py3? @dpiparo
There was a problem hiding this comment.
There is no problem running the tutorials with python 3 from the command line.
The issue has to do with scoping of variables when the tutorial is run from within the doxygen filter. The global scope is then the makeimage.py script and not the tutorial. So variables declared as "global" in the tutorial are no longer global when run inside the makeimage wrapper, and therefore not accessible in a local scope (like inside a function definition).
In this case the "RDataFrame = ROOT.ROOT.RDataFrame" earlier in the file is not visible inside the scope of fill_tree().
|
|
||
| def WithPyROOT(): | ||
| def WithPyROOT(filename): | ||
| from math import sqrt |
There was a problem hiding this comment.
Why moving the import inside the function?
There was a problem hiding this comment.
Same here. The import if done at the file scope is not visible in the local scope of WithPyROOT() when the tutorial is run inside the makeimage wrapper.
| @@ -38,7 +38,12 @@ | |||
| 3.0)) | |||
|
|
|||
| # For each quantile fill with the pdf | |||
| xx = [-1.5] + [quant.GetBinContent(i)for i in range(1, 9)] + [1.5] | |||
There was a problem hiding this comment.
Why eliminating the (nicer) list comprehension syntax?
There was a problem hiding this comment.
This is the same, but python 3 specific. The list comprehension in Python 3 defines the code that is run for each element as a function call, and hence in a new local scope. So it can not see the quant variable when run inside the makeimage wrapper.
There was a problem hiding this comment.
With the other proposed changes in this PR applied (so that python 3 is supported in the doxygen filter) except this rewrite of the list comprehension in the tStudent.py tutorial you get the following error when running the doxygen filter on the file:
$ DOXYGEN_SOURCE_DIRECTORY=../.. DOXYGEN_OUTPUT_DIRECTORY=/tmp/x PYTHON_EXECUTABLE=/usr/bin/python3 ./filter ../../tutorials/math/tStudent.py >/dev/null
[NbConvertApp] Converting notebook /tmp/x/notebooks/tStudent.py.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: python3
[NbConvertApp] Writing 26589 bytes to /tmp/x/notebooks/tStudent.py.nbconvert.ipynb
Info in <ROOT::Math::MathMoreLibrary>: libMathMore has been loaded.
Traceback (most recent call last):
File "makeimage.py", line 40, in <module>
makeimage(argv[1], argv[2], argv[3], bool(argv[4]), bool(argv[5]), bool(argv[6]))
File "makeimage.py", line 15, in makeimage
if py: exec(compile(open(MacroName, "rb").read(), MacroName, 'exec'))
File "../../tutorials/math/tStudent.py", line 41, in <module>
xx = [-1.5] + [quant.GetBinContent(i)for i in range(1, 9)] + [1.5]
File "../../tutorials/math/tStudent.py", line 41, in <listcomp>
xx = [-1.5] + [quant.GetBinContent(i)for i in range(1, 9)] + [1.5]
NameError: name 'quant' is not defined
Segmentation fault (minnesutskrift skapad)
With the change it works as expected:
$ DOXYGEN_SOURCE_DIRECTORY=../.. DOXYGEN_OUTPUT_DIRECTORY=/tmp/x PYTHON_EXECUTABLE=/usr/bin/python3 ROOTSYS= ./filter ../../tutorials/math/tStudent.py >/dev/null
[NbConvertApp] Converting notebook /tmp/x/notebooks/tStudent.py.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: python3
[NbConvertApp] Writing 26667 bytes to /tmp/x/notebooks/tStudent.py.nbconvert.ipynb
Info in <ROOT::Math::MathMoreLibrary>: libMathMore has been loaded.
TH1::TH1:0: RuntimeWarning: nbins is <=0 - set to nbins = 1
Info in <TCanvas::Print>: png file /tmp/x/html/pict1_tStudent.py.png has been created
| def WithRDataFrameVecOpsJit(): | ||
| f = RDF(treename, filename) | ||
| def WithRDataFrameVecOpsJit(treename, filename): | ||
| f = ROOT.ROOT.RDataFrame(treename, filename) |
There was a problem hiding this comment.
Same comment as above here as well. @dpiparo
|
You can see in the following pages that the images are missing: https://root.cern/doc/v616/df003__profiles_8py.html So the issue with the scope is not due to the proposed changes to support python 3. |
|
Hi @ellert , thank you for the explanations! I would go ahead with it, but first I would add an explanation to the commit message about the change of scope when running with the |
When run inside the makeimage.py wrapper during doxygen generation the variable scope is different than when the tutorials are run stand-alone. The change in tutorials/math/tStudent.py is Python 3 specific and is due to that list comprehensions in Python 3 are run in an implicit function scope.
ellert
force-pushed
the
doxygen-with-python-3
branch
from
08e49c5
to
dd4b1d6
Compare
|
I have split the PR into two commits, and extended the commit message for the commit that changes the tutorials. |
|
Ping? |
|
Hi Enric, is this PR ok for you on the python side ? |
|
Yes, for me it's good to go. |
|
Ok I merge |
This PR fixes problems when building the doxygen documentation when the ROOT Python bindings (PyROOT) are built against Python 3.