Skip to content
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

[PyROOT] Do __getattr__ pythonization of TTree in Python #14441

Closed
wants to merge 2 commits into from

Conversation

guitargeek
Copy link
Contributor

There is no need to do this on the C++ size.

@guitargeek guitargeek self-assigned this Jan 25, 2024
@guitargeek guitargeek force-pushed the ttree_getattr branch 2 times, most recently from ccc9aea to 4bf0ad6 Compare January 25, 2024 19:15
@pcanal
Copy link
Member

pcanal commented Jan 25, 2024

@smuzaffar Could you check if this break any CMSSW code? Actually more interesting is: #14444

@guitargeek
Copy link
Contributor Author

@smuzaffar Could you check if this break any CMSSW code? Actually more interesting is: #14444

It doesn't matter which one you test, they both contain the commit with the GetAddress() and GetObject() return type

Copy link

github-actions bot commented Jan 26, 2024

Test Results

    12 files      12 suites   2d 19h 41m 44s ⏱️
 2 646 tests  2 643 ✅ 0 💤  3 ❌
30 025 runs  29 991 ✅ 0 💤 34 ❌

For more details on these failures, see this check.

Results for commit ed7d772.

♻️ This comment has been updated with latest results.

@smuzaffar
Copy link
Contributor

@smuzaffar Could you check if this break any CMSSW code? Actually more interesting is: #14444

@pcanal , cmssw tests are running via cms-sw#196

@smuzaffar
Copy link
Contributor

mostly looks good for cmssw. Only one unit test failed with error [a]

[a] https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce122a/37051/unitTests/src/DQMServices/Demo/test/TestDQMServicesDemo/testing.log

++ /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-25-2300/src/DQMServices/Demo/test/dqmiodumpentries.py alltypes.root -r 1 --summary
Traceback (most recent call last):
  File "/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-25-2300/src/DQMServices/Demo/test/dqmiodumpentries.py", line 54, in <module>
    value = metree.Value
  File "/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/lcg/root/6.31.01-29afad78d0e3ad51f539f0164a6c7494/lib/ROOT/_pythonization/_ttree.py", line 266, in _TTree__getattr__
    proxy = bind_branch_to_proxy(self, name, branch)
  File "/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/lcg/root/6.31.01-29afad78d0e3ad51f539f0164a6c7494/lib/ROOT/_pythonization/_ttree.py", line 212, in bind_branch_to_proxy
    return cppyy.bind_object(branch.GetAddress()[0], branch.GetClassName())
TypeError: 'nullptr_t' object is not subscriptable
+ '[' '0: 1, 0.0: 1, 1: 11, 100: 33, 200: 11, 5: 16, 5.0: 5' = '' ']'

---> test TestDQMServicesDemo had ERRORS

@guitargeek
Copy link
Contributor Author

Thank you very much for catching these errors, @smuzaffar. It appears our CI doesn't cover all code branches...

Anyway, given that the pythonization implemented in C++ still works fine even after the CPyCppyy upgrade in my other PR, this translation to Python is not necessary at this moment, and I will close this PR.

@guitargeek guitargeek closed this Mar 16, 2024
@guitargeek guitargeek deleted the ttree_getattr branch March 16, 2024 18:29
@guitargeek guitargeek restored the ttree_getattr branch June 7, 2024 13:29
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@root-project root-project deleted a comment from phsft-bot Jun 7, 2024
@guitargeek guitargeek reopened this Jun 7, 2024
@guitargeek guitargeek force-pushed the ttree_getattr branch 2 times, most recently from 1990713 to ed7d772 Compare June 11, 2024 08:06
So far, the `TBranch::GetAddress` method returned a `char *`. This
causes incompatibility with PyROOT, because `char *` is automatically
converted to a Python string, which is the wrong thing to for an
address.

This made the return value unusable in PyROOT. That's a bug that
currently prevents us from reimplementing some pythonizations in native
Python.

Therefore, the return type of `TBranch::GetAddress()` was changed to
`void *` in this release. Even if this change is not backwards
compatible for typing reasons, the returned value is the same.

With this change, the code is also more consistent because
`GetAddress()` and `SetBranchAddress()` use the same type for the
address.
There is no need to do this on the C++ size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants