key4hep / EDM4hep

Generic event data model for HEP collider experiments
https://cern.ch/edm4hep
Apache License 2.0
24 stars 35 forks source link

Any attribute can be set for members of collections from Python #288

Closed jmcarcell closed 4 months ago

jmcarcell commented 4 months ago

It seems to be harmless:

import edm4hep
coll = edm4hep.MCParticleCollection()
p = coll.create()
p.HELLO = "HELLO"
print(p.HELLO) # 'HELLO'

but it's bad when there is a typo from an existing member and it will not report any errors. You can even mistype the field everywhere and it won't be a problem unless the collection is saved to a file and then the field you expected isn't there. I don't know what can be done to change this since the python bindings come from ROOT. The same is also true for collections themselves.

m-fila commented 4 months ago

__slots__ can be used to explicitly declare the members. I'm not sure if there is a nice way to do it with cppyy. Maybe using some clever pythonizations?

Example with slots:

>>> class Foo:
...     __slots__ = ["bar"]
...     def __init__(self, bar):
...         self.bar = bar
... 
>>> foo = Foo(0)
>>> foo.bar = 42
>>> foo.baz = 44
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Foo' object has no attribute 'baz'
jmcarcell commented 4 months ago

I'm not sure how that would play with how ROOT loads its members, and of course doing this for every class would be very ugly. I asked in the cppyy repo: https://github.com/wlav/cppyy/issues/222 I think this is the intended behavior, let's see if there is a way of modifying it.

jmcarcell commented 4 months ago

Answered in https://github.com/wlav/cppyy/issues/222 It seems I haven't stumbled before on setting arbitrary attributes on instances on classes, so I didn't know that was normal Python. I don't think then anything is going to be done about this; adding something for every class we export in Python from C++ would be ugly.