JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

No SetSingle for JMultiFactory #226

Closed DraTeots closed 10 months ago

DraTeots commented 1 year ago

At least in non PODIO version

nathanwbrei commented 1 year ago

Technically there is no SetSingle in JFactoryT, either. JFactoryT::Insert(T*) serves the purpose of SetSingle, however the name Insert implies you can call it multiple times to insert data one item at a time, which PODIO resoundingly rejects. I'd rather not be constantly explaining to users why JMultifactory::Insert works for non-PODIO data and throws an exception for PODIO datatypes. We could add a SetDatum(std::string tag, T* datum) but I'm worried this gives users too many things that are similar but slightly different to have to think about. What argument in favor of adding this did you have in mind?

nathanwbrei commented 1 year ago

I guess we could technically accumulate everything passed to Insert() into a std::vector, and not add it to a PODIO collection until JMultifactory::Process finishes. This will be annoying to implement and test though. Is it worth it?

faustus123 commented 1 year ago

Could we just throw the exception for PODIO types, but give it a long-ish message explaining that it is a limitation of PODIO and therefore not allowed for PODIO data types? This should save you from having to explain it. I'm only suggesting this if it is a quick/easy thing to implement.

DraTeots commented 1 year ago

What I don't like about it is that there is like 2 JANA2-s, one with PODIO and one without. They behave differently and their user expectations are different. It basically forces you to write different code for both (I am not touching event model here). Initially it looked like implementation compromise but as it goes further it starts to look like major architectural flow.

nathanwbrei commented 10 months ago

Moving this discussion to #254