JeffersonLab / JANA2

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

GetNumObjects is always 0 for factories created in multifactories #275

Closed DraTeots closed 3 days ago

DraTeots commented 4 months ago

If factory is produced by specifying the output of multifactory its GetNumObjects == 0 all the time. It is better illustrated by this code:

for (auto factory: event->GetFactorySet()->GetAllFactories()) {
   const std::string &obj_name = factory->GetObjectName();   // F125Cluster
   auto obj_num = factory->GetNumObjects();   // (!) <== always 0
   auto f125_clusters = event->Get<ml4fpga::fpgacon::F125Cluster>();   // returns 15 objects

The real problem I am facing behind this bug is that in real events some EVIO information might not exist for some events. This leads to a case when some chain of factories are not producing results. So in upstream Processor I have to check what types are there.

It might be that GetNumObjects is just misleading. GetNumObjects shows always 0 because the factory hasn't produced data yet. One can deduce which absence of EVIO data would lead to which factory chain crashes. But the problem is that upstream Processor should know all implementation details, while by design its just asking the data it needs, right? So how to check that?

faustus123 commented 4 months ago

@nathanwbrei will probably want to comment here, but let me at least help hone in on the issue.

It is true that in JANA2 the GetNumObjects() method is only intended to return the number of current objects in the factory without invoking any creation of them. There is another method, GetCreationStatus() that can be used to check if the factory has been run already. One can force a factory to try and create the objects first by calling Create(event).

So if the goal is to have obj_num in your code always represent the number of objects assuming the factory has been run, then I see a couple of options:

  1. Add a call in your code to factory->Create() just before calling factory->GetNumObjects()
  2. Modify Create() to return the number of objects so you do it in one call.
  3. Add a flag to GetNumObjects() that allows the user to specify whether Create() should be called automatically and you again do it in one call.

I may also be missing something about multifactory that is the true root of the issue.

nathanwbrei commented 4 months ago

Like David said, GetNumObjects() doesn't trigger Create(). This is very much on purpose -- the main reason we need GetNumObjects() is so we can distinguish between these different cases:

I don't remember exactly why Create() returns void instead of numObjects like JANA1 did, but we could easily change that back without breaking anything. Of the three options David lists, I prefer (2).

While you are at it, could you quickly test the following just to make sure there isn't a multifactory bug?

for (auto factory: event->GetFactorySet()->GetAllFactories()) {
   const std::string &obj_name = factory->GetObjectName();   // F125Cluster
   auto obj_num = factory->GetNumObjects();   // (!) <== always 0
   auto f125_clusters = event->Get<ml4fpga::fpgacon::F125Cluster>();   // returns 15 objects
   obj_num = factory->GetNumObjects();   // SHOULD BE 15 NOW

One thing I'm working on right now is improving our handling of missing/optional values from inside OmniFactories and JEventProcessors. If you are around the week I'd love to hear more about your use case for GetNumObjects().

DraTeots commented 4 months ago

I'll check that!

For the record, OmniFactories are completely brocken for outside of PodIO... But I create another bug report for that

nathanwbrei commented 4 months ago

Thanks for the heads-up. Go ahead and file the bug report. I was aware that the non-podio template wouldn't even instantiate, but I thought Wouter or Dmitry K. fixed it... No problem, I have a bunch of other small fixes coming on that front

DraTeots commented 4 months ago

... but I thought Wouter or Dmitry K. fixed it...

Yeah, mee too. So I wanted to talk in general of such approach and not about particular small bugs

nathanwbrei commented 3 days ago

Closing this. After looking into it closely, I decided against modifying Create() to return the number of objects created. At first I was worried because it adds another virtual method call, which could slow down every single call to JEvent::Get. A bigger problem is that it directly conflicts with some ongoing refactoring work for #276. Calling Create() followed by GetNumObjects() is the preferred way to go here.