Open nathanwbrei opened 10 months ago
There are several different places we can go from here. Some of these are mutually exclusive.
Insert()
s and Set()
s to SetData()
with appropriate overloads, and drop the questionable insert semantics to begin with. mData
. In EICrecon, we set a convention of only accessing PODIO data as collections. Maybe this convention should be enforced in the API as well?I'm not sure if I'm following this completely. It seems the PODIO immutable collection issue places some practical limitations on what can be done with inserts from the JANA side. I would not want to lose the ability to Insert() single objects or vectors of objects into an event for non-PODIO applications. We may need to discuss it in person if it is more complicated than that.
Yeah, this is a "big" discussion. Not happening overnight.
@faustus123 In terms of functionality we would lose if we pursued (0), it's not much:
Calling JEvent::Insert
Calling JEvent::InsertCollection() and JEvent::InsertCollectionAlreadyInFrame() would have the exact same functionality, they would just be renamed to Set*, which makes more sense because multiple inserts cannot happen.
JFactoryT::Insert() would be replaced with a JFactoryT::SetData
This aggregates a number of earlier complaints about part of our API. These all need to be solved at the same time, and would constitute a major breaking change.
JEventSources are supposed to call JEvent::Insert(), but JFactories and JEventProcessors are not. Weirdly, JEvent::Insert is incorrectly marked
const
(#155). (This is probably a workaround from the days before we had Multifactories)JEvent provides two versions of Insert(), one which "inserts" a whole collection into a dummy factory, and one of which "inserts" one item into an existing collection in a dummy factory. The latter is (a) inefficient when there are multiple inserts and (b) not compatible with PODIO's immutable collection semantics. In theory the former JEvent::Insert() works with PODIO data, although we strongly encourage PODIO users to use InsertCollection() and InsertCollectionAlreadyInFrame() instead.
JFactoryT's are supposed to call JFactoryT::Set() for collections and JFactoryT::Insert() for individual items. (Note this is inconsistent with JEventSources, where you call JEvent::Insert for collections as well; see issue #154) JFactoryPodioT's, on the other hand, provide JFactoryT::Set() and JFactoryT::Insert() (which actually has SetSingle() semantics since you can't append to a PODIO collection after publishing it to the Frame) but also SetCollection(), which is what users are really supposed to be using instead.
JMultifactories are only supposed to call SetData() or SetCollection(). There is no corresponding Insert()/SetSingle(). (#226)
Overall, I'd say it mostly makes sense how we got here. However, the mismatch between the JANA+PODIO parts and the PODIO-free parts is definitely higher than I'd like, and naming things "Insert" some of the time (particularly when they don't have full insert semantics) was a mistake.