root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.65k stars 1.26k forks source link

Iterators in pyROOT working differently in ROOT master compared to 6.30/02 #15269

Closed will-cern closed 1 month ago

will-cern commented 5 months ago

Check duplicate issues.

Description

I've had a report that some behaviour of pyROOT for iterating over a class is not working in ROOT master but is working in ROOT 6.30

Here's the correct behaviour ... note the TYPE of the object in the for loop, it is another xRooNode, as it should be.

>>> import ROOT
>>> from ROOT.Experimental import XRooFit
>>> w = XRooFit.xRooNode("RooWorkspace","w","w"); w.Add("factory:mu[1]")
xRooNode{}
>>> for c in w: print(type(c))
... 
<class cppyy.gbl.ROOT.Experimental.XRooFit.xRooNode at 0x8d0bd20>
>>> ROOT.gROOT.GetVersion()
'6.30/02'

Same code in ROOT master:

>>> import ROOT
>>> from ROOT.Experimental import XRooFit
>>> w = XRooFit.xRooNode("RooWorkspace","w","w"); w.Add("factory:mu[1]")
xRooNode{}
>>> for c in w: print(type(c))
... 
<class 'cppyy.LowLevelView'>
>>> ROOT.gROOT.GetVersion()
'6.33/01'

Is it known why the iteration functionality is different?

Reproducer

see above

ROOT version

ROOT master

Installation method

source /cvmfs/sft.cern.ch/lcg/views/dev3/latest/x86_64-el9-gcc13-opt/setup.sh

Operating system

lxplus el9

Additional context

No response

will-cern commented 5 months ago

Just found that the problem appears in 6.30/04 as well, so at least can narrow down the change to between 6.30/02 and 6.30/04 .... tested with source /cvmfs/sft.cern.ch/lcg/views/LCG_105a/x86_64-el9-gcc13-opt/setup.sh (6.30/04) vs source /cvmfs/sft.cern.ch/lcg/views/LCG_105/x86_64-el9-gcc13-opt/setup.sh (6.30/02)

guitargeek commented 5 months ago

Thanks for reporting! Maybe this is a duplicate of this issue: https://github.com/root-project/root/issues/15234

I need to check what happens when porting the fix in CPyCppyy to ROOT.

will-cern commented 5 months ago

So I actually found that the problem can be fixed by reverting some changes we made to the xRooNodeIterator back in December, where we were trying to fix a cppcheck error about returning a temporary. The problem is in fact a one line fix in xRooNode.h, replacing:

decltype(auto) operator*() const

with

const std::shared_ptr<xRooNode>& operator*() const

This makes cppcheck unhappy again, but the problem is fixed. I've actually now asked about this cppcheck error on https://stackoverflow.com/questions/78349719 and waiting to see if anyone responds - so far one person suggests it could be a false positive.

aaronj0 commented 1 month ago

Hi @will-cern

Looks like the issue no longer exists with the latest PyROOT updates:

(root_venv) ➜  ROOT python
Python 3.12.3 (main, Jul 31 2024, 17:43:48) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ROOT
>>> from ROOT.Experimental import XRooFit
>>> w = XRooFit.xRooNode("RooWorkspace","w","w"); w.Add("factory:mu[1]")
xRooNode{}
>>> for c in w: print(type(c))
... 
<class cppyy.gbl.ROOT.Experimental.XRooFit.xRooNode at 0x906dac0>
>>> ROOT.gROOT.GetVersion()
'6.33.01'
>>> 

Closing as this issue is fixed.

github-actions[bot] commented 1 month ago

Hi @aaronj0, @guitargeek,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely, :robot: