optimise cache usage and reduce class size #67

AuroraPerego · 2025-03-19T17:02:10Z

BEGINRELEASENOTES

add operator<<, reorder class members and do not return const& for simple types in CLUECalorimeterHit
use std::vector<T> instead of std::vector<T>* and add the cluster index to each hit in the CLUENtuplizer
use the file downloaded when building the repo in clue_gaudi_wrapper.py

ENDRELEASENOTES

jmcarcell · 2025-03-25T16:14:08Z

gaudi_opts/clue_gaudi_wrapper.py

@@ -27,11 +27,14 @@
 from Configurables import PodioOutput
 from Configurables import ApplicationMgr

+from os import environ
+build_path = environ["ENV_CMAKE_BINARY_DIR"]


This part is making CI fail. It should not be part of the script, at least not as it is. Setting a default name is fine but a default file with a hardcoded path won't be useful for someone else. You can pass the input file as it is being done now in the tests with --EventDataSvc.input in https://github.com/key4hep/k4Clue/blob/main/src/k4clue/CMakeLists.txt#L45.

I thought this would be general enough since the file is the one downloaded for the tests and the directory is the build directory, but it seems that in the CI the env variable is not set. I've commented it.

jmcarcell · 2025-03-27T15:05:14Z

include/CLUECalorimeterHit.h


 private:
-  std::uint8_t m_detectorRegion{0};
-  std::uint64_t m_layer{};


I don't know the details but this changed from 64 bits to 32 bits, I guess the layer can not be bigger than 32 bits?

layer goes up to 40 right now (80 if we sum the endcap)

jmcarcell · 2025-03-27T15:05:49Z

include/CLUENtuplizer.h

-    delete m_clhits_z;
-    delete m_clhits_energy;
-  };
+  ~CLUENtuplizer(){};


Suggested change

~CLUENtuplizer(){};

If defining the destructor is not needed let's not define it since it's default one.

ok, i'll change it

jmcarcell · 2025-03-27T15:08:01Z

src/CLUENtuplizer.cpp

@@ -98,6 +98,7 @@ StatusCode CLUENtuplizer::execute(const EventContext&) const {
  } else {
    throw std::runtime_error("CLUE hits collection not available");
  }
+  const auto& clue_calo_coll_vect = clue_calo_coll->vect;


This seems to be used only below when computing clusInBarrel, then maybe move it down there to right before when it's needed so that the definition is close to where it's used?

yes, sure! I probably had some code in the middle that I removed

jmcarcell · 2025-03-27T15:10:29Z

gaudi_opts/clue_gaudi_wrapper.py

+#from os import environ
+#build_path = environ["ENV_CMAKE_BINARY_DIR"]
+


Suggested change

#from os import environ

#build_path = environ["ENV_CMAKE_BINARY_DIR"]

Remove instead of introducing commented out code? I think this may be because you are running this steering file locally? But an argument can be passed in the command line as I mentioned above which is even faster than setting the env variable and commenting this in.

ok sure, no problem

jmcarcell · 2025-03-27T15:14:25Z

src/CLUENtuplizer.cpp

+      });
+  uint32_t offset = 0;
+  if (clusInBarrel != clue_calo_coll_vect.end() && clusInBarrel->inBarrel())
+    offset = clusInBarrel->getClusterIndex() + 1;


This wasn't here before. Can you explain, for example in the release notes, what effect does this have or what is this used for? (Used later in m_hits_clusId?

that is a way to obtain the maximum cluster index in the barrel and add this offset to the endcap (thinking about it maybe I can do it in a better way..)
Hits from barrel and endcap are clustered in two separated steps, so we'll have clusters with indices 0, 1 .. in the barrel and 0, 1.. in the endcap. With this offset, the clusters indices will be unique.
I can add it to the release notes

I'll postpone this change to the PR where I'll also remove the if statement that now prevents saving events where there is more than one MC Particle.
With that if only events with one of few clusters are saved, so this fix is not really needed.

jmcarcell mentioned this pull request Mar 23, 2025

add a .clang-format file and format the repo #66

Closed

restructure CLUECalorimeterHit and add operator <<

5f36f52

AuroraPerego force-pushed the refactoring branch from 4681594 to 28fb415 Compare March 24, 2025 09:42

jmcarcell reviewed Mar 25, 2025

View reviewed changes

AuroraPerego force-pushed the refactoring branch from 28fb415 to 61c0b0a Compare March 25, 2025 16:29

jmcarcell reviewed Mar 27, 2025

View reviewed changes

AuroraPerego added 2 commits March 28, 2025 12:08

do not use raw pointers for vectors

d5a7916

add cluster index to hits

03f9910

AuroraPerego force-pushed the refactoring branch from 61c0b0a to 03f9910 Compare March 28, 2025 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimise cache usage and reduce class size #67

optimise cache usage and reduce class size #67

AuroraPerego commented Mar 19, 2025

jmcarcell Mar 25, 2025

AuroraPerego Mar 25, 2025

jmcarcell Mar 27, 2025

AuroraPerego Mar 27, 2025

jmcarcell Mar 27, 2025

AuroraPerego Mar 27, 2025

jmcarcell Mar 27, 2025

AuroraPerego Mar 27, 2025

jmcarcell Mar 27, 2025

AuroraPerego Mar 27, 2025

jmcarcell Mar 27, 2025

AuroraPerego Mar 27, 2025 •

edited

Loading

AuroraPerego Mar 28, 2025

		#from os import environ
		#build_path = environ["ENV_CMAKE_BINARY_DIR"]

optimise cache usage and reduce class size #67

Are you sure you want to change the base?

optimise cache usage and reduce class size #67

Conversation

AuroraPerego commented Mar 19, 2025

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

AuroraPerego Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AuroraPerego Mar 27, 2025 •

edited

Loading