-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add python bindings for writing Frames #447
Conversation
57c7a63
to
56b3472
Compare
This became quite a big bigger than I initially anticipated, since there seem to be some quirks around how cppyy selects overloads for the parameters part. Given the improvements in the overall "pythonness" that we now have in EDM4hep, I am no longer sure the user visible import structure we have here is the best, but I would put that into another PR. |
@tmadlener - so this PR is complete as is? |
Technically yes. I have to check why it is failing CI. Also assuming that we address a nicer "import interface" later. |
""" | ||
if not _is_collection_base(collection): | ||
raise ValueError("Can only put podio collections into a Frame") | ||
return self._frame.put(cppyy.gbl.std.move(collection), name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried this but after this using collection
probably crashes right? It's not trivial to see this if you don't have the C++ background
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it and indeed it crashes, for this PR is fine but there should be something to prevent those...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on how you want to use the collection. Indexing into it will crash, because it is effectively empty. Adding new things to it might work, but I haven't tried it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a bit of playing things seem to work until you try to write a collection a second time, i.e.
event = Frame()
hits = ExampleHitCollection()
# add hits
frame.put(hits, "Hits")
len(hits) # == 0
hits.create()
len(hits) # == 1
frame.put(hits, "moreHits") # seg-fault
I will update the documentation that collections should not be used after calling put
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like here going out of range may be more of a problem than in C++ (this is not related to this PR but just indexing out of range a std::deque
):
event = Frame()
hits = ExampleHitCollection()
hits[0] # seg fault
hits[1] # <cppyy.gbl.MutableExampleHit object at 0x55dcd5d30d50>
hits[1].x() # seg fault
hits.create(0, 1, 2. 3, 4)
hits[0] # <cppyy.gbl.MutableExampleHit object at 0x55dcc4324040>
hits[1] # <cppyy.gbl.MutableExampleHit object at 0x55dcd5d572b0>
hits[2] # seg fault
It's inconsistent since the behavior is undefined, could be a bit hard to debug when you go out of range and it may seem to work but then it doesn't later on. I'm not sure if there are options, not checking in C++ I think is a design choice since there is also .at
, but in python no one is going to use that probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in the current form there are not that many handrails in python. Could probably be fixed by another thin python shim that introduces the range check. That could then also be used to raise an error in the use-after-move case.
Created #447 to keep track of this.
python/podio/frame.py
Outdated
@@ -163,6 +196,46 @@ def _get_param_value(par_type, name): | |||
|
|||
return _get_param_value(vec_types[0], name) | |||
|
|||
def put_parameter(self, key, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the overload for floats will always be double, is it worth it to add a float=True
that will select the float overload? Otherwise it's impossible to add a float parameter from python, in the rare case someone needs float and not double...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible now via an additional as_type
argument.
bb5ee42
to
bdf118e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The interface can be improved like in EDM4hep but that can be done later.
BEGINRELEASENOTES
put
method to theFrame
wrapper that allows to add collections to the Frame without having to explicitly usecppyy.gbl.std.move
. Fixes Improve python interface when adding collections to a frame #432ENDRELEASENOTES
put_parameters
(+ tests)