[Exp PyROOT] Add item setting pythonisation for TClonesArray #3423
Conversation
|
Starting build on |
| TObject *object = (*cla)[index]; | ||
| pyobj->CppOwns(); | ||
| MemoryRegulator::RegisterPyObject(pyobj, object); | ||
| memcpy((void *)object, pyobj->GetObject(), cla->GetClass()->Size()); |
There was a problem hiding this comment.
In general this is not a safe way of copying the objects. [It might work in many cases but many other will leave dangling pointers in the target]. See TObject::Copy which might be the solution here.
See also TClonesArray::ConstructedAt and TClonesArray::Absorb (i.e. or maybe we need a special case for pyroot added to TClonesArray).
There was a problem hiding this comment.
I'm trying to use TObject::Copy instead of memcpy but it seems to have no effect, the destination object remains null. I tried a quick test:
root [0] TObjString o("a");
root [1] TObjString o2("b");
root [2] cout << o.GetName() << endl;
a
root [3] cout << o2.GetName() << endl;
b
root [4] o.Copy(o2)
root [5] cout << o.GetName() << endl;
a
root [6] cout << o2.GetName() << endl;
b
I would expect o to be copied into o2 but that does not seem the case. What I am missing? Thanks!
There was a problem hiding this comment.
that TObjString does not overload Copy .. :( ... so indeed we can't rely on it ....
There was a problem hiding this comment.
The 'solution' is to use/implement a variant of TDirectory::CloneObject that replace the first statement (calling New()) with an input parameter ....
|
Starting build on |
| Py_INCREF(obj); | ||
| PyObject *result = PyObject_CallMethod(obj, const_cast<char *>(meth), const_cast<char *>("O"), arg1); | ||
| PyObject* result = PyObject_CallMethod(obj, const_cast<char*>(meth), const_cast<char*>("")); | ||
| Py_DECREF(obj); |
There was a problem hiding this comment.
Why do we need the increment and decrement of the reference count around the call method?
There was a problem hiding this comment.
Good point, I don't think we do!
| // To know the capacity of a TClonesArray, we need to invoke GetSize | ||
| PyObject *pysize = CallPyObjMethod(self, "GetSize"); | ||
| if (!pysize) { | ||
| PyErr_Clear(); |
There was a problem hiding this comment.
Which error is thrown if you clear the error here?
There was a problem hiding this comment.
I will set a custom error message for it, thanks!
| return nullptr; | ||
| } | ||
|
|
||
| PyObject *pyindex = nullptr; |
There was a problem hiding this comment.
Do you need to increment the count here? You could just do a pass-through?
There was a problem hiding this comment.
The caller of PyStyleIndex is responsible for decrefing the return value (pyindex). pyindex can be either index (refcount 2) or the result of PyLong_FromSsize_t (refcount 1). In either case, the caller (SetItem) will decref, so everything is fine. I will add a comment about this!
etejedor
force-pushed
the
tclonesarraypyz
branch
from
b869970
to
adbde9b
Compare
|
Starting build on |
The `TClonesArray.__setitem__` pythonization was introduced in commit 67db9e5 with PR root-project#3423. However, this pythonization is quite unnatural, because a `TObjArray` should not adopt external memory. In fact, on in the C++ side, all `TObjArray`-inherited methods that take a `TObject *` and that would adopt these objects are flagged as `MayNotUse` for good reasons! Supporting this element setting only on the Python side therefore comes with a very high cost of hacking deep into CPyCppyy, and the benefit of this is not clear because the PR that introduced this feature didn't refer to any user request. Hence, this commit suggests to remove this pythonization, with the ultimate goal of relying less on the usage of CPyCppyy internals.
The `TClonesArray.__setitem__` pythonization was introduced in commit 67db9e5 with PR root-project#3423. However, this pythonization is quite unnatural, because a `TObjArray` should not adopt external memory. In fact, on in the C++ side, all `TObjArray`-inherited methods that take a `TObject *` and that would adopt these objects are flagged as `MayNotUse` for good reasons! Supporting this element setting only on the Python side therefore comes with a very high cost of hacking deep into CPyCppyy, and the benefit of this is not clear because the PR that introduced this feature didn't refer to any user request. Hence, this commit suggests to remove this pythonization, with the ultimate goal of relying less on the usage of CPyCppyy internals.
The `TClonesArray.__setitem__` pythonization was introduced in commit 67db9e5 with PR root-project#3423. However, this pythonization is quite unnatural, because a `TObjArray` should not adopt external memory. In fact, on in the C++ side, all `TObjArray`-inherited methods that take a `TObject *` and that would adopt these objects are flagged as `MayNotUse` for good reasons! Supporting this element setting only on the Python side therefore comes with a very high cost of hacking deep into CPyCppyy, and the benefit of this is not clear because the PR that introduced this feature didn't refer to any user request. Hence, this commit suggests to remove this pythonization, with the ultimate goal of relying less on the usage of CPyCppyy internals.
The `TClonesArray.__setitem__` pythonization was introduced in commit 67db9e5 with PR #3423. However, this pythonization is quite unnatural, because a `TObjArray` should not adopt external memory. In fact, on in the C++ side, all `TObjArray`-inherited methods that take a `TObject *` and that would adopt these objects are flagged as `MayNotUse` for good reasons! Supporting this element setting only on the Python side therefore comes with a very high cost of hacking deep into CPyCppyy, and the benefit of this is not clear because the PR that introduced this feature didn't refer to any user request. Hence, this commit suggests to remove this pythonization, with the ultimate goal of relying less on the usage of CPyCppyy internals.
The pythonisation proposed in this PR injects a
__setitem__implementation intoTClonesArraythat customizes the setting of an item.The
__setitem__pythonization thatTClonesArrayinherits fromTSeqCollectiondoes not apply in this case and a redefinition is required. The reason isTClonesArraysets objects by constructing them in-place, which is impossible to support as the Python object given as value must exist a priori. It can, however, be memcpy'd and stolen, which is the approach used in this redefinition. This is also the reason why this pythonisation needs to be implemented in C++.