linux-eic-shell.yml: produce reco hits, not just copy raw hits in eicrecon-two-stage-running #1071

veprbl · 2023-10-11T19:43:26Z

Copying over the raw EDM4hep hits is a bit too convenient. The intention was to test the analysis continuation.

veprbl · 2023-10-11T20:37:08Z

As expected, this doesn't actually work:

Thread model: pthreads
140506247313088: 
       `- JSignalHandler::produce_overall_report[abi:cxx11]() (0x7fca6b9b6e6c)
        `- JSignalHandler::handle_sigsegv(int, siginfo_t*, void*) (0x7fca6b9b7ff6)
         `- /lib/x86_64-linux-gnu/libc.so.6 (0x7fca6905afd0)
          `- eicrecon::CalorimeterHitDigi::process(edm4hep::SimCalorimeterHitCollection const&) (0x7fca51372564)
           `- eicrecon::CalorimeterHitDigi_factoryT::Process(std::shared_ptr<JEvent const> const&) (0x7fca526c293e)
            `- JMultifactoryHelperPodio<edm4hep::RawCalorimeterHit>::Process(std::shared_ptr<JEvent const> const&) (0x7fca525c7d10)
             `- JFactory::Create(std::shared_ptr<JEvent const> const&) (0x7fca6b904cf8)
              `- JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) (0x7fca527327a5)
               `- JEvent::GetCollectionBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) const (0x7fca5b725ee2)
                `- eicrecon::CalorimeterHitReco_factoryT::Process(std::shared_ptr<JEvent const> const&) (0x7fca526c5a7a)
                 `- JMultifactoryHelperPodio<edm4eic::CalorimeterHit>::Process(std::shared_ptr<JEvent const> const&) (0x7fca525c81e0)
                  `- JFactory::Create(std::shared_ptr<JEvent const> const&) (0x7fca6b904cf8)
                   `- JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) (0x7fca5272b6d5)
                    `- JEvent::GetCollectionBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) const (0x7fca5b725ee2)
                     `- eicrecon::CalorimeterIslandCluster_factoryT::Process(std::shared_ptr<JEvent const> const&) (0x7fca526caac6)
                      `- JMultifactoryHelperPodio<edm4eic::ProtoCluster>::Process(std::shared_ptr<JEvent const> const&) (0x7fca525c86b0)
                       `- JFactory::Create(std::shared_ptr<JEvent const> const&) (0x7fca6b904cf8)
                        `- JFactoryPodioT<edm4eic::ProtoCluster>::Create(std::shared_ptr<JEvent const> const&) (0x7fca527245bf)
                         `- JEvent::GetCollectionBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) const (0x7fca5b725ee2)
                          `- eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) (0x7fca526cf5cd)
                           `- JMultifactoryHelperPodio<edm4eic::Cluster>::Process(std::shared_ptr<JEvent const> const&) (0x7fca525c8b80)
                            `- JFactory::Create(std::shared_ptr<JEvent const> const&) (0x7fca6b904cf8)
                             `- JFactoryPodioT<edm4eic::Cluster>::Create(std::shared_ptr<JEvent const> const&) (0x7fca527449f1)
                              `- JEvent::GetCollectionBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) const (0x7fca5b725ee2)
                               `- eicrecon::EnergyPositionClusterMerger_factoryT::Process(std::shared_ptr<JEvent const> const&) (0x7fca4f7b3228)
                                `- JMultifactoryHelperPodio<edm4eic::MCRecoClusterParticleAssociation>::Process(std::shared_ptr<JEvent const> const&) (0x7fca525c9050)
                                 `- JFactory::Create(std::shared_ptr<JEvent const> const&) (0x7fca6b904cf8)
                                  `- JFactoryPodioT<edm4eic::MCRecoClusterParticleAssociation>::Create(std::shared_ptr<JEvent const> const&) (0x7fca5273a068)
                                   `- JEvent::GetCollectionBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) const (0x7fca5b725ee2)
                                    `- JEventProcessorPODIO::Process(std::shared_ptr<JEvent const> const&) (0x7fca419885ac)
                                     `- JEventProcessor::DoMap(std::shared_ptr<JEvent const> const&) (0x7fca6b93e72d)
                                      `- JEventProcessorArrow::execute(JArrowMetrics&, unsigned long) (0x7fca6b926625)
                                       `- JWorker::loop() (0x7fca6b931437)
                                        `- /lib/x86_64-linux-gnu/libstdc++.so.6 (0x7fca69ad44a3)
                                         `- /lib/x86_64-linux-gnu/libc.so.6 (0x7fca690a8044)
                                          `- /lib/x86_64-linux-gnu/libc.so.6 (0x7fca691285fc)

simonge · 2023-10-13T12:48:33Z

Copying over the raw EDM4hep hits is a bit too convenient. The intention was to test the analysis continuation.

Is it not the other way around. EcalBarrelScFiHits and EcalBarrelImagingHits are the EDM4hep "raw" hits while EcalBarrelScFiRawHits and EcalBarrelImagingRawHits are the post digitization step hits.

The problem here will be that the digitization step requires the associations with an edm4hep::CaloHitContributions collection which hasn't been written out

veprbl · 2023-10-13T15:25:07Z

@simonge You must be right. I've misinterpreted a message about missing to mean that we were missing a reconstruction step
https://github.com/eic/EICrecon/actions/runs/6488263828/job/17620869065?pr=1069#step:7:518
But it's actually an explicit input for Associations

EICrecon/src/detectors/BEMC/BEMC.cc

Lines 79 to 84 in 46f7d9a

    
           new JChainMultifactoryGeneratorT<CalorimeterClusterRecoCoG_factoryT>( 
        
              "EcalBarrelScFiClusters", 
        
             {"EcalBarrelScFiProtoClusters",        // edm4eic::ProtoClusterCollection 
        
              "EcalBarrelScFiHits"},                // edm4hep::SimCalorimeterHitCollection 
        
             {"EcalBarrelScFiClusters",             // edm4eic::Cluster 
        
              "EcalBarrelScFiClusterAssociations"}, // edm4eic::MCRecoClusterParticleAssociation

I guess, that should mean that we can't do such test with included Associations, which is something we might have to face later.

kkauder · 2023-10-13T15:43:42Z

@simonge Correct. It would be nice to rename the simple Hits into SimuHits or something like that to avoid confusion. RawHits are the objects that should be treated the same whether coming from digitized simulations or from real data.

simonge · 2023-10-13T15:49:44Z

It's in the digitization factory where the associations aren't explicit and probably should be.

EICrecon/src/detectors/BEMC/BEMC.cc

Lines 27 to 30 in 46f7d9a

    
           app->Add(new JChainMultifactoryGeneratorT<CalorimeterHitDigi_factoryT>( 
        
              "EcalBarrelScFiRawHits", 
        
              {"EcalBarrelScFiHits"}, 
        
              {"EcalBarrelScFiRawHits"},

Using the contributions:

EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc

Line 122 in 46f7d9a

for (const auto& c : hit.getContributions()) {

simonge · 2023-10-13T16:45:52Z

There are two issues this has brought to light although not directly critical for this test.

Continued analysis of a collection requiring it's associations needs the associations to also be saved - can/should this be forced? Or might this result in everything being linked to each other anyway and saved whether asked for or not.
The association collection doesn't need to exist for the code to try and access it. For this example the error could be caught if the association collection is an input to the relevant calorimetry factories.

Actually, both suggestions are flawed due to the large network of associations required...

wdconinc · 2023-10-13T23:48:46Z

In the days of juggler, this was addressed with flexibility by allowing truth associations to be optional. If they weren't in the input, an algorithm wouldn't produce output truth associations.

veprbl · 2023-10-14T19:34:35Z

In the days of juggler, this was addressed with flexibility by allowing truth associations to be optional. If they weren't in the input, an algorithm wouldn't produce output truth associations.

Can you elaborate? Was the optionality implemented in python? Was it implemented conditionally on presence of certain input collections?

wdconinc · 2023-10-14T20:00:56Z

Defining an input or output collection in gaudi is by string of the collection name, just like other properties could be strings. When a string was passed for associations, we just got the data handle for that collection; if not, then no collections were used. See e.g. https://eicweb.phy.anl.gov/EIC/juggler/-/blob/v8.0.2/JugReco/src/components/ClusterRecoCoG.cpp?ref_type=tags#L116

Algorithms has a similar interface which allows for optional collections, see e.g. https://github.com/eic/algorithms/blob/main/calorimetry/src/ClusterRecoCoG.cpp#L56 (mind you, the input and output types are pointers and the optional ones can be nullptr).

I think a similar interface should be easily supportable in EICrecon if we allow the factory to catch the right exception, e.g. JException:: CollectionNotFound, and set the pointer to nullptr (and throw for other exceptions as we do now).

wdconinc · 2023-10-14T20:02:23Z

It's not just something that would be nice to have, but it's a requirement that we are able to run without MC associations, as soon as we get test beam that we want to analyze with EICrecon, in fact.

veprbl · 2023-10-14T22:58:25Z

It's not just something that would be nice to have, but it's a requirement that we are able to run without MC associations, as soon as we get test beam that we want to analyze with EICrecon, in fact.

We are already able to run without MC associations - the factories that throw an exception are automatically excluded from writing (the logic is in the JEventProcessorPODIO).

Defining an input or output collection in gaudi is by string of the collection name, just like other properties could be strings. When a string was passed for associations, we just got the data handle for that collection; if not, then no collections were used. See e.g. https://eicweb.phy.anl.gov/EIC/juggler/-/blob/v8.0.2/JugReco/src/components/ClusterRecoCoG.cpp?ref_type=tags#L116

So, I can see that a missing collection doesn't disable the collection, but leads to algorithm not populating the DataHandle in ::execute(). I don't know if in JANA production of a collection is a requirement for successful execution of a factory with the same tag, but, I suspect, those two things may be not tied together (it might be that it returns nullptr from GetCollectionBase, like you suggest below). Then the same could be implemented in EICrecon. Then the question is how to handle missing data in the downstream factories? We would need to instrument them to also skip work if associations are not available.

I think a similar interface should be easily supportable in EICrecon if we allow the factory to catch the right exception, e.g. JException:: CollectionNotFound, and set the pointer to nullptr (and throw for other exceptions as we do now).

Yeah, if we find that using exceptions for control flow is the JANA way, we might restrict the types of those that are used for that purpose.

wdconinc · 2023-10-14T23:00:41Z

Probably a topic for discussion with @nathanwbrei at one of the next Wednesday meetings.

veprbl · 2024-01-13T20:38:47Z

This was invalid, so let's close.

linux-eic-shell.yml: produce reco hits, not just copy raw htis

3505d8d

github-actions bot added the topic: infrastructure label Oct 11, 2023

veprbl temporarily deployed to github-pages October 11, 2023 22:54 — with GitHub Actions Inactive

veprbl mentioned this pull request Jan 13, 2024

JEventProcessorPODIO: treat all unhandled exceptions as fatal #1069

Open

veprbl closed this Jan 13, 2024

veprbl deleted the pr/eicrecon-two-stage-running_use_rec_hits branch January 13, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linux-eic-shell.yml: produce reco hits, not just copy raw hits in eicrecon-two-stage-running #1071

linux-eic-shell.yml: produce reco hits, not just copy raw hits in eicrecon-two-stage-running #1071

veprbl commented Oct 11, 2023

veprbl commented Oct 11, 2023

simonge commented Oct 13, 2023 •

edited

Loading

veprbl commented Oct 13, 2023

kkauder commented Oct 13, 2023

simonge commented Oct 13, 2023

simonge commented Oct 13, 2023 •

edited

Loading

wdconinc commented Oct 13, 2023

veprbl commented Oct 14, 2023

wdconinc commented Oct 14, 2023

wdconinc commented Oct 14, 2023

veprbl commented Oct 14, 2023

wdconinc commented Oct 14, 2023

veprbl commented Jan 13, 2024

linux-eic-shell.yml: produce reco hits, not just copy raw hits in eicrecon-two-stage-running #1071

linux-eic-shell.yml: produce reco hits, not just copy raw hits in eicrecon-two-stage-running #1071

Conversation

veprbl commented Oct 11, 2023

veprbl commented Oct 11, 2023

simonge commented Oct 13, 2023 • edited Loading

veprbl commented Oct 13, 2023

kkauder commented Oct 13, 2023

simonge commented Oct 13, 2023

simonge commented Oct 13, 2023 • edited Loading

wdconinc commented Oct 13, 2023

veprbl commented Oct 14, 2023

wdconinc commented Oct 14, 2023

wdconinc commented Oct 14, 2023

veprbl commented Oct 14, 2023

wdconinc commented Oct 14, 2023

veprbl commented Jan 13, 2024

simonge commented Oct 13, 2023 •

edited

Loading

simonge commented Oct 13, 2023 •

edited

Loading