Skip to content

Conversation

@etejedor
Copy link
Collaborator

This PR mainly adds pythonisations for TCollection and its subclasses. It also adds a pythonisation for TIter, which is necessary to iterate over TCollections.

The corresponding unit tests for every pythonisation are also proposed.

@etejedor etejedor self-assigned this Jan 22, 2019
@etejedor etejedor requested review from dpiparo and stwunsch January 22, 2019 12:42
@etejedor etejedor requested a review from amadio as a code owner January 22, 2019 12:42
@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

@etejedor
Copy link
Collaborator Author

@phsft-bot build just on ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3 with flags -Dpyroot_experimental=ON

@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3 with flags -Dpyroot_experimental=ON
How to customize builds

@dpiparo
Copy link
Member

dpiparo commented Jan 22, 2019

@phsft-bot build just on ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3 with flags -Dpyroot_experimental=ON -DCTEST_TEST_EXCLUDE_NONE=ON

@phsft-bot
Copy link

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3 with flags -Dpyroot_experimental=ON -DCTEST_TEST_EXCLUDE_NONE=ON
How to customize builds

@dpiparo
Copy link
Member

dpiparo commented Jan 22, 2019

just wanted to try out...

Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

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

Great improvement. I suggested some changes. Do we perhaps need tutorials for this?

# - c: collection to extend self with
lenc = c.GetEntries()
it = TIter(c)
for i in range(lenc):
Copy link
Member

Choose a reason for hiding this comment

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

good bot!

# Python-list-like methods

def _remove_pyz(self, o):
# Parameters:
Copy link
Member

Choose a reason for hiding this comment

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

alignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

# - Number of occurrences of the object in the collection
n = 0

it = TIter(self)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe using range iteration (root collections have a begin and an end) is simpler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you are suggesting, how are begin and end useful from Python?

# - self: collection to be iterated
# Returns:
# - TIter iterator on collection
return TIter(self)
Copy link
Member

Choose a reason for hiding this comment

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

Just as a curiosity: if you use just begin() and return the stl iterator, it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean returning self.begin() here?

# - c: collection to extend self with
lenc = c.GetEntries()
it = TIter(c)
for i in range(lenc):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for i in range(lenc):
for _ in range(lenc):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

# Make TIter a Python iterator.
# This makes it possible to iterate over TCollections, since Cppyy
# injects on them an `__iter__` method that returns a TIter.
klass.__next__ = _next_pyz # Py3
Copy link
Member

Choose a reason for hiding this comment

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

do we need both for both versions or we can make the two cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is quite common to just assign both to the same function, and it does not harm anyway, see e.g. :
https://portingguide.readthedocs.io/en/latest/iterators.html#new-iteration-protocol-next

@etejedor etejedor merged commit c78c0f7 into root-project:master Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants