podio::DataSource #309

kjvbrt · 2023-08-07T13:31:36Z

DataSource providing EDM4hep collections. This allows writing analyzers using EDM4hep collections directly.
Example:

    struct selPDG {
      selPDG(const int pdgID, const bool chargeConjugateAllowed = false);
      const int m_pdg;
      const bool m_chargeConjugateAllowed;
      edm4hep::TrackCollection operator() (
          const edm4hep::MCRecoTrackParticleAssociationCollection& inAssocColl);
    };

Two implementations are provided, frame and legacy.
To run the analysis using these analyzers --use-source or --use-legacy-source.

Implementation: The class keeps three vectors m_Collections, m_podioReaders and m_frames.
Thread safety: questionable

vvolkl · 2023-08-14T07:50:13Z

This will be very nice to have, thanks! I'll take a closer look this week. For the naming, I'd suggest "DataSource" instead of "Source". This should also eventually move to the edm4hep repository, right?

kjvbrt · 2023-08-15T09:28:45Z

Thanks for the suggestion I renamed the Source to DataSource.

tmadlener

Very nice. I have started to have a look and I have made some more or less detailed comments below. I also have a few questions about how this is supposed to work in the end for RDataFrame, and about some of the concepts there.

First a conceptual one: Does a slot in RDataFrame correspond to a thread? Following up on that, IIUC the basic idea here is to have one reader per slot / thread and then also one Frame per slot / thread? I am not entirely sure how much overhead this is memory wise, but it might be possible to only have one reader but multiple Frames. However, I am not entirely sure how easy it would be to make this work with the RDF infrastructure.

Then one from the usage side. In the end this would be used as free functions that get "proper" edm4hep collections as arguments and return new (subset) collections? e.g.

df.Define("reco_pt", "getPt(ReconstructedParticles)")

or would the idea be to have them as sort of "member functions" in the end? (Is that even possible?)

Then finally one that already goes beyond this PR, but sort of fits anyhow: Can the Snapshot mechanism be made to produce EDM4hep output again from this?

tmadlener · 2023-08-15T12:37:38Z

e4hsource/include/EDM4hepDataSource/EDM4hepDataSource.hxx

+      EDM4hepDataSource(const std::vector<std::string>& filePathList,
+                        int nEvents = -1);
+
+      void SetNSlots(unsigned int nSlots);


Can the virtual functions here be marked with either final or override, just so that the compiler has a chance to detect mismatches here?

tmadlener · 2023-08-15T12:45:11Z

e4hsource/include/EDM4hepDataSource/EDM4hepDataSource.hxx

+      std::vector<std::unique_ptr<podio::ROOTFrameReader>> m_podioReaders;
+      /// Podio frames
+      std::vector<std::unique_ptr<podio::Frame>> m_frames;


I don't know how RDataFrame handles this internally and which parts need to be shared over several threads, and which are managed by RDataFrame and we basically can assume that it will only be accessed from only one thread.

From a podio point of view:

ROOTFrameReader / ROOTLegacyReader: no internal thread safety at all, i.e. either make sure that each thread has it's own reader or put a lock / mutex around it for readEntry/readNextEntry.

Frame: fully thread safe. Access from as many thread as you want, including putting new things into it. With the caveat that this uses a lock / mutex internally, so there could be some lock congestion if used from very many threads

tmadlener · 2023-08-15T12:48:30Z

e4hsource/src/EDM4hepDataSource.cxx

+    if (std::find(m_columnNames.begin(),
+                  m_columnNames.end(),
+                  columnName) != m_columnNames.end()) {
+      return true;
+    }
+
+    return false;


Suggested change

if (std::find(m_columnNames.begin(),

m_columnNames.end(),

columnName) != m_columnNames.end()) {

return true;

}

return false;

return std::find(m_columnNames.begin(),

m_columnNames.end(),

columnName) != m_columnNames.end();

Could ditch the if here and simply return the result directly.

tmadlener · 2023-08-15T12:49:23Z

e4hsource/src/EDM4hepDataSource.cxx

+                << m_columnTypes.at(i) << std::endl;
+
+      return m_columnTypes.at(i) + "Collection";


Suggested change

<< m_columnTypes.at(i) << std::endl;

return m_columnTypes.at(i) + "Collection";

<< m_columnTypes[i] << std::endl;

return m_columnTypes[i] + "Collection";

No need to do another range-check here, since we have already done that just above.

tmadlener · 2023-08-15T12:58:31Z

analyzers/dataframe/FCCAnalyses/ReconstructedParticle.h

+  ROOT::VecOps::RVec<float>
+  getPt(const edm4hep::ReconstructedParticleCollection& inParticles);


This is most likely beyond the scope if this pull request, but there is probably quite a bit of (kinematic) functionality that could be templated on the collection type and then simply forwarded to other utility functionality, e.g. the one that already comes with EDM4hep

tmadlener · 2023-08-15T12:59:38Z

analyzers/dataframe/FCCAnalyses/ReconstructedParticle.h

+  struct selPDG {
+    selPDG(const int pdgID, const bool chargeConjugateAllowed = false);
+    const int m_pdg;
+    const bool m_chargeConjugateAllowed;
+    edm4hep::ReconstructedParticleCollection operator() (
+        const edm4hep::MCRecoParticleAssociationCollection& inAssocColl);
+  };


Also this is most likely beyond the scope if this PR, but this also looks general enough to be templated (but I a likely missing a detail here).

Zehvogel · 2024-04-11T07:01:16Z

Can we split the DataSource itself off of this and get it into edm4hep directly? I tested it a bit and it looked ready to use?

tmadlener · 2024-04-11T07:31:12Z

If putting the datasource into EDM4hep would be (more or less) easily possible, I would also be in favor of having it there. That should make maintenance a bit easier and we would see earlier if we break it when we do changes in EDM4hep.

tmadlener

I only had a quick look at some of the files and made a few smaller comments.

Overall, what is the (longer term) plan for FCCAnalysis for switching to the podio datasource? This PR suggests that at least for some time the "old way" and the "new way" will coexist for some time. Since EDM4hep has changed enough such that newer files cannot be easily read with older versions of FCCAnalysis, I am wondering whether it makes sense to have the two things in parallel, or whether it would be easier to simply make a clean cut and move everything to the data source way.

Some of the functionality might also be general enough for it to be moved to EDM4hep? E.g. getting the pt (or four momentum) for any datatype that has a momentum (and at least a mass or energy).

tmadlener · 2024-09-19T12:16:03Z

CMakeLists.txt

+set(WITH_SOURCE ON CACHE STRING "Enable PODIO Data Source")
+set_property(CACHE WITH_SOURCE PROPERTY STRINGS ON OFF)


Suggested change

set(WITH_SOURCE ON CACHE STRING "Enable PODIO Data Source")

set_property(CACHE WITH_SOURCE PROPERTY STRINGS ON OFF)

set(WITH_PODIO_DATASOURCE ON CACHE STRING "Enable PODIO Data Source")

set_property(CACHE WITH_PODIO_DATASOURCE PROPERTY STRINGS ON OFF)

Just to give this a slightly more discernible name.

Good point, used WITH_PODIO_DATASOURCE

tmadlener · 2024-09-19T12:18:20Z

analyzers/dataframe/src/AssociationSource.cc

+  template <typename T>
+  T sel_PDG::operator() (const T& inAssocColl) {
+    T result;
+    result.setSubsetCollection();
+
+    for (const auto& assoc: inAssocColl) {
+      const auto& particle = assoc.getSim();
+      if (particle.getPDG() == m_pdg) {
+        result.push_back(assoc);
+      }
+    }
+
+    return result;
+  }


This definition needs to go to the header file, otherwise we will most likely run into linker errors. Unless, there are explicit template instantiations that I am missing somewhere. Similar for others cases.

Moved also with other ones.

tmadlener · 2024-09-19T12:22:23Z

analyzers/dataframe/FCCAnalyses/ReconstructedParticleSource.h

+     *                     with the MC particle with the desired PDG ID.
+     */
+    edm4hep::ReconstructedParticleCollection operator() (
+        const edm4hep::MCRecoParticleAssociationCollection& inAssocColl);


Suggested change

const edm4hep::MCRecoParticleAssociationCollection& inAssocColl);

const edm4hep::RecoMCParticleLinkCollection& inAssocColl);

The Association types are deprecated.

Everything was converted into Links.

tmadlener · 2024-09-19T12:24:51Z

analyzers/dataframe/src/ReconstructedParticleSource.cc

+      lVec.SetXYZM(particle.getMomentum().x,
+                   particle.getMomentum().y,
+                   particle.getMomentum().z,
+                   particle.getMass());


This could be done via edm4hep::utils::p4(particle, edm4hep::utils::UseMass) from the edm4hep/utils/kinematics.h header.

Thanks for the hint, used...

tmadlener · 2024-09-19T12:25:41Z

analyzers/dataframe/src/ReconstructedParticleSource.cc

+        return std::sqrt(std::pow(p.getMomentum().x, 2) +
+                         std::pow(p.getMomentum().y, 2));
+      }


Similar here edm4hep::utils::pt can also do this.

kjvbrt · 2024-09-20T13:48:26Z

I only had a quick look at some of the files and made a few smaller comments.

Thanks, this was just work in progress commit...

Overall, what is the (longer term) plan for FCCAnalysis for switching to the podio datasource? This PR suggests that at least for some time the "old way" and the "new way" will coexist for some time. Since EDM4hep has changed enough such that newer files cannot be easily read with older versions of FCCAnalysis, I am wondering whether it makes sense to have the two things in parallel, or whether it would be easier to simply make a clean cut and move everything to the data source way.

I would like to ditch "old way" at some point, but I'm not sure we can make clean cut quickly. There is a lot of analysis code which uses the "old way" and also the performance of the datasource way needs to be better understood.

Some of the functionality might also be general enough for it to be moved to EDM4hep? E.g. getting the pt (or four momentum) for any datatype that has a momentum (and at least a mass or energy).

Upstreaming some of the functionalities to edm4hep is the good point. How would you handle generating of those functions? With C++ templating or something like Podio(jinja2)?
Long term I thing it would be good to separate c++ libraries from Python orchestration and only at the point where we know which files the analysis will operate on we source appropriate stack.

Zehvogel · 2024-09-20T14:16:15Z

also the performance of the datasource way needs to be better understood.

I think unfortunately the performance will always be worse. Podio gives us an AoS style access in memory while when we access the root file directly we have SoA. I.e. using the podio datasource will potentially always result in reading also unneeded data. Unless I am confusing something... Maybe @m-fila wants to comment? :)

tmadlener · 2024-09-20T14:26:10Z

There is a lot of analysis code which uses the "old way" and also the performance of the datasource way needs to be better understood.

Good point. Didn't think of that.

I think unfortunately the performance will always be worse. Podio gives us an AoS style access in memory while when we access the root file directly we have SoA. I.e. using the podio datasource will potentially always result in reading also unneeded data. Unless I am confusing something... Maybe @m-fila wants to comment? :)

There are a few things to consider. The major difference is that the podio data source currently has no way to only read selective branches / collections only. It will always read the full event. On top of that reconstructing the relations takes a bit of time. At some point that might be an acceptable overhead, but how large the analysis has to be for that is unclear to me. There are also almost certainly some optimizations that can be (or are already) done if the podio middleman is cut out of the whole thing. The memory layout is another thing, but that might even be something we can change in podio, because the interface should give us enough abstraction to switch the implementation below from AoS to SoA, but this has to be investigated further.

analyzers/dataframe/FCCAnalyses/LinkSource.h

analyzers/dataframe/FCCAnalyses/ReconstructedParticleSource.h

analyzers/dataframe/src/ReconstructedParticleSource.cc

kjvbrt requested review from vvolkl and tmadlener August 8, 2023 13:54

kjvbrt changed the title ~~[WIP] EDM4hep Source~~ [WIP] EDM4hep DataSource Aug 14, 2023

kjvbrt force-pushed the source branch from c4be0b1 to 32b6d36 Compare August 15, 2023 08:41

tmadlener reviewed Aug 15, 2023

View reviewed changes

kjvbrt force-pushed the source branch from 1f5169c to 1416be0 Compare October 26, 2023 13:33

kjvbrt force-pushed the source branch from 5ad051d to 815dfeb Compare January 25, 2024 14:23

kjvbrt force-pushed the source branch from 2306b47 to 77dfa99 Compare March 1, 2024 15:39

kjvbrt force-pushed the source branch from 57b05a6 to 7708cbb Compare May 3, 2024 07:28

kjvbrt force-pushed the source branch from a46fc37 to 62f833b Compare August 27, 2024 14:29

kjvbrt changed the title ~~[WIP] EDM4hep DataSource~~ podio::DataSource Sep 4, 2024

kjvbrt force-pushed the source branch from cdc974b to 93188c1 Compare September 18, 2024 15:00

tmadlener reviewed Sep 19, 2024

View reviewed changes

kjvbrt mentioned this pull request Sep 20, 2024

Release 2024-09-20ish key4hep/key4hep-spack#647

Closed

19 tasks

tmadlener reviewed Sep 20, 2024

View reviewed changes

kjvbrt added 8 commits October 1, 2024 10:02

Integrating EDM4hep RDataSource

b6d1734

Add legacy reader

264899a

Split off legacy reader

93fd3ac

Protecting collection retrieval by mutex

a2172d0

Add source test

58292c8

Updated man pages

96600f5

Renaming Source na DataSource

33a357f

Adding source test for run stages

ca7e91e

kjvbrt added 21 commits October 1, 2024 10:02

Add tests for the standalone mode

ccd3671

Adding test files per process for histmaker

da73dbb

Adding selector by size and sorter by pT

616a827

Few more analyzers

2187e48

Putting back things omitted from run_analysis.py

a61b6cf

Adding e4hsource in LD_LIBRARY_PATH for managed tests

1530e5e

Making building of the source optional

285a170

Removed legacy support

e1aa165

Separated analyzers using Collections into independent headers

9d74103

Reimplementing analyzers needed for the stage1 of the example analysis

258813c

Adjusitng test input files

311379b

Moving recoParticle definition

27dcbbd

Changes to use podio::ROOTDataSource

97db64b

using podio::DataSource

b1b76c3

Adjusting stages source example

ce2981f

Adding C++ analysis tests

005ac4e

Updating examples

53f3402

Formatting

7bd35ce

Removing e4hsource

d1aad33

Adjust clang-format check

c2aa199

Formatting

eb6dffe

kjvbrt force-pushed the source branch from fa1e7a4 to eb6dffe Compare October 1, 2024 08:03

kjvbrt added 3 commits October 1, 2024 14:31

Using PodioSource namespace

026ff54

Adding .cache to .gitignore

c0eb383

Formatting

26240f1

kjvbrt merged commit eef66b3 into HEP-FCC:master Oct 1, 2024
3 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podio::DataSource #309

podio::DataSource #309

kjvbrt commented Aug 7, 2023 •

edited

Loading

vvolkl commented Aug 14, 2023

kjvbrt commented Aug 15, 2023 •

edited

Loading

tmadlener left a comment

tmadlener Aug 15, 2023

tmadlener Aug 15, 2023

tmadlener Aug 15, 2023

tmadlener Aug 15, 2023

tmadlener Aug 15, 2023

tmadlener Aug 15, 2023

Zehvogel commented Apr 11, 2024

tmadlener commented Apr 11, 2024

tmadlener left a comment

tmadlener Sep 19, 2024

kjvbrt Sep 20, 2024

tmadlener Sep 19, 2024

kjvbrt Sep 20, 2024

tmadlener Sep 19, 2024

kjvbrt Sep 20, 2024

tmadlener Sep 19, 2024

kjvbrt Sep 20, 2024

tmadlener Sep 19, 2024

kjvbrt Sep 20, 2024

kjvbrt commented Sep 20, 2024 •

edited

Loading

Zehvogel commented Sep 20, 2024 •

edited

Loading

tmadlener commented Sep 20, 2024

		<< m_columnTypes.at(i) << std::endl;

		return m_columnTypes.at(i) + "Collection";

		ROOT::VecOps::RVec<float>
		getPt(const edm4hep::ReconstructedParticleCollection& inParticles);

		set(WITH_SOURCE ON CACHE STRING "Enable PODIO Data Source")
		set_property(CACHE WITH_SOURCE PROPERTY STRINGS ON OFF)

	const edm4hep::MCRecoParticleAssociationCollection& inAssocColl);
	const edm4hep::RecoMCParticleLinkCollection& inAssocColl);

podio::DataSource #309

podio::DataSource #309

Conversation

kjvbrt commented Aug 7, 2023 • edited Loading

vvolkl commented Aug 14, 2023

kjvbrt commented Aug 15, 2023 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zehvogel commented Apr 11, 2024

tmadlener commented Apr 11, 2024

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjvbrt commented Sep 20, 2024 • edited Loading

Zehvogel commented Sep 20, 2024 • edited Loading

tmadlener commented Sep 20, 2024

kjvbrt commented Aug 7, 2023 •

edited

Loading

kjvbrt commented Aug 15, 2023 •

edited

Loading

kjvbrt commented Sep 20, 2024 •

edited

Loading

Zehvogel commented Sep 20, 2024 •

edited

Loading