Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use new MetaDataHandles to be compliant with Frame based I/O #41

Merged
merged 2 commits into from
Jun 28, 2023
Merged

Use new MetaDataHandles to be compliant with Frame based I/O #41

merged 2 commits into from
Jun 28, 2023

Conversation

tmadlener
Copy link
Contributor

key4hep/k4FWCore#100 makes the default DataSvc use the Frame, and some of the interfaces for accessing meta data (e.g. cellID strings) will change. This PR changes the access / writing of the cell ID encoding strings to use the new MetaDataHandle interface.

@tmadlener tmadlener mentioned this pull request May 25, 2023
Merged
@tmadlener tmadlener changed the title [WIP] Use new MetaDataHandles to be compliant with Frame based I/O Use new MetaDataHandles to be compliant with Frame based I/O Jun 27, 2023
kjvbrt
kjvbrt reviewed Jun 28, 2023
View reviewed changes
RecFCCeeCalorimeter/src/components/CreateCaloCellPositionsFCCee.h Outdated
@@ -91,10 +92,10 @@ class CreateCaloCellPositionsFCCee : public GaudiAlgorithm {
dd4hep::DDSegmentation::BitFieldCoder* m_decoder = new dd4hep::DDSegmentation::BitFieldCoder("system:4");
/// Input collection
DataHandle<edm4hep::CalorimeterHitCollection> m_hits{"hits/hits", Gaudi::DataHandle::Reader, this};
MetaDataHandle<std::string> m_hitsCellIDEncoding{m_hits, "CellIDEncodingString", Gaudi::DataHandle::Reader, this};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should MetaDataHandle take fatherAlg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no. It seems like I didn't update this PR after that has been removed again from key4hep/k4FWCore#100. Let me fix that

kjvbrt
kjvbrt reviewed Jun 28, 2023
View reviewed changes
RecFCCeeCalorimeter/src/components/CreateCaloCellPositionsFCCee.h Outdated
/// Output collection
DataHandle<edm4hep::CalorimeterHitCollection> m_positionedHits{"hits/positionedHits", Gaudi::DataHandle::Writer, this};
PodioDataSvc* m_podioDataSvc;
ServiceHandle<IDataProviderSvc> m_eventDataSvc;
MetaDataHandle<std::string> m_positionedHitsCellIDEncoding{m_positionedHits, "CellIDEncodingString", Gaudi::DataHandle::Reader, this};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

@kjvbrt
Copy link
Contributor

kjvbrt commented Jun 28, 2023 •
edited
Loading

The PR builds locally with the new frame I/O

@kjvbrt kjvbrt merged commit 4704fb6 into HEP-FCC:main Jun 28, 2023
@giovannimarchiori
Copy link
Contributor

Hello! Was it checked that the code would run in addition to building successfully? I managed to compile but when running fccrun with the usual script (https://github.com/BrieucF/LAr_scripts/blob/main/FCCSW_ecal/runTopoAndSlidingWindowAndCaloSim.py) I get a FATAL error:

CalBarrelPosit... FATAL Exception with tag=Put for non-writing MetaDataHandle not allowed is caught
ECalBarrelPosit... ERROR Put for non-writing MetaDataHandle not allowed MetaDataHandle policy violation StatusCode=FAILURE

@tmadlener
Copy link
Contributor Author

tmadlener commented Jul 17, 2023 •
edited
Loading

This probably broke here: #43

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 17, 2023

The #43 was intended to fix the issue. The m_positionedHitsCellIDEncoding should be output metadata handle no?

@giovannimarchiori
Copy link
Contributor

Ah, I hadn't pulled #43 yet. Indeed that fixes the error I see with the metadata, though the code fails later in SimG4SaveCalHits

TUnixSystem::Di... FATAL segmentation violation

===========================================================
There was a crash.
This is the entire stack trace of all threads:

#0 0x00007f7a4e51829a in wait4 () from /lib64/libc.so.6
#1 0x00007f7a4e46195b in do_system () from /lib64/libc.so.6
#2 0x00007f7a4cce3fd5 in TUnixSystem::Exec (shellcmd=, this=0x1f6f550) at /tmp/root/spack-stage/spack-stage-root-6.28.04-jf5p3cc3fsy3oqln2re7ovtmux5phrup/spack-src/core/unix/src/TUnixSystem.cxx:2104
#3 TUnixSystem::StackTrace (this=0x1f6f550) at /tmp/root/spack-stage/spack-stage-root-6.28.04-jf5p3cc3fsy3oqln2re7ovtmux5phrup/spack-src/core/unix/src/TUnixSystem.cxx:2395
#4 0x00007f7a4cce1785 in TUnixSystem::DispatchSignals (this=0x1f6f550, sig=kSigSegmentationViolation) at /tmp/root/spack-stage/spack-stage-root-6.28.04-jf5p3cc3fsy3oqln2re7ovtmux5phrup/spack-src/core/unix/src/TUnixSystem.cxx:3615
#5
#6 0x00007f7a2ed44f89 in SimG4SaveCalHits::saveOutput(G4Event const&) () from /home/gmarchio/work/fcc-new/k4SimGeant4/install/lib/libSimG4Components.so

@tmadlener
Copy link
Contributor Author

Ah right. My bad then. Are there some simple examples that could be put together into a minimal test suite?

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 17, 2023

The segmentation violation issue should be fixed by HEP-FCC/k4SimGeant4#40.

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 17, 2023

With HEP-FCC/k4SimGeant4#40 the test https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/runCaloSim.py passes, but consequent one https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/runFullCaloSystem_ReconstructionSW_noiseFromFile.py fails with no obvious error. But this looks suspicious:

PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::CalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::ClusterCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::ClusterCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::CalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::SimCalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::CalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::SimCalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::CalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::SimCalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::MCParticleCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::CalorimeterHitCollection
PODIO WARNING: evolveBuffers has no knowledge of how to evolve buffers for edm4hep::SimCalorimeterHitCollection

@tmadlener
Copy link
Contributor Author

The warnings can be ignored for now. This is effectively a small issue in the lookup logic in podio. However, since there is no schema evolution to do at the moment, everything works, even with these warnings. We can silence them if we have sorted things out if necessary.

@giovannimarchiori
Copy link
Contributor

giovannimarchiori commented Jul 20, 2023 •
edited
Loading

The segmentation violation issue should be fixed by HEP-FCC/k4SimGeant4#40.

Indeed, I merged the changes of that MR in my local fork and I have been able to run the simulation successfully in a recent nightly and produce the same hits and cells as with older nightlies and the legacy podio interface
HEP-FCC/k4SimGeant4@main...giovannimarchiori:k4SimGeant4:gmarchio-main-ModuleThetaReadout

@kjvbrt kjvbrt mentioned this pull request Sep 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjvbrt kjvbrt kjvbrt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tmadlener @kjvbrt @giovannimarchiori