API inconsistency: Set vs Insert vs SetSingle

[nathanwbrei]

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 (Public access to JEvent::Set #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 JEvent::Set and JFactory::Insert #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(). (No SetSingle for JMultiFactory #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.

[nathanwbrei]

There are several different places we can go from here. Some of these are mutually exclusive.

0. We can rename ALL Insert()s and Set()s to SetData() with appropriate overloads, and drop the questionable insert semantics to begin with.
1. We can attempt to add support for true Insert() semantics for PODIO types by maintaining a mutable collection under the hood, and only handing it over to the podio Frame after JFactory::Process() has finished.
2. We could give up supporting accessing PODIO data using classical JANA Get/Set. If we did this, JFactoryPodioT would no longer need to inherit from JFactoryT (eliminating these weird Set and Insert calls the users probably shouldn't use anyhow) and we would have a lot fewer headaches 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?
3. Maybe we could just upgrade everybody to using Omnifactories and no longer have ANY user calls to Set or Insert. I would be weirdly happy doing this. One thing about the GlueX port that bothers me is that the JANA2 API doesn't look any nicer (and in some respects looks worse) than the JANA1 API it is replacing. Omnifactories are genuinely nicer to look at

[faustus123]

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.

[nathanwbrei]

Yeah, this is a "big" discussion. Not happening overnight.

[nathanwbrei]

@faustus123 In terms of functionality we would lose if we pursued (0), it's not much:

• Calling JEvent::Insert(T*, tag) from JEventSource::GetEvent multiple times entices users but is very inefficient, because it has to look up the dummy factory from the factory set each time. Constraining users to either call JEvent::SetData(T*, tag) exactly once, or create their own std::vector and pass that in via JEvent::SetData(std::vector<T*>&, tag) seems like an improvement to me.

• 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(T* item), which can only be called once, and for multiple items the user would have to create their own std::vector and call JFactoryT::SetData(std::vector<T*> items). I actually liked JFactoryT::Insert() because it lets me push items into mData without referencing mData directly, and without creating an intermediate std::vector<T*>. It has one weird flaw of its own, however. In the GlueX code there's some factories where lots of little methods push items directly into mData, and do a sort over it at the end. Accessing mData directly is necessary for that unless we want to do a heavy-duty refactor. So if Insert() doesn't provide enough access into mData, its role might be better served by something like std::vector<T*>& GetData().