Skip to content

Conversation

starsdong
Copy link

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

@veprbl
Copy link
Member

veprbl commented Jun 13, 2025

Here is a script to help you test this in EICrecon:

#!/usr/bin/env bash

set -xue

export CMAKE_INSTALL_PREFIX=$PWD/prefix
git clone git@github.com:eic/EDM4eic.git -b pr/helix_utils
pushd EDM4eic
cmake -S . -B build -DBUILD_SHARED_LIBS=ON
cmake --build build -- -j $(nproc)
cmake --install build
popd
git clone git@github.com:eic/EICrecon.git
pushd EICrecon/
git apply <<EOF
diff --git a/cmake/jana_plugin.cmake b/cmake/jana_plugin.cmake
index 8fde2d8c..7fd1be04 100644
--- a/cmake/jana_plugin.cmake
+++ b/cmake/jana_plugin.cmake
@@ -369,7 +369,7 @@ macro(plugin_add_event_model _name)
   # Add libraries (same as target_include_directories but for both plugin and
   # library)
   plugin_link_libraries(\${PLUGIN_NAME} podio::podio EDM4EIC::edm4eic
-                        EDM4HEP::edm4hep)
+                        EDM4HEP::edm4hep edm4eic_helix_utils)
 
 endmacro()
 
diff --git a/src/algorithms/tracking/ActsToTracks.cc b/src/algorithms/tracking/ActsToTracks.cc
index 947299b4..ce18d6f3 100644
--- a/src/algorithms/tracking/ActsToTracks.cc
+++ b/src/algorithms/tracking/ActsToTracks.cc
@@ -11,6 +11,7 @@
 #include <edm4eic/EDM4eicVersion.h>
 #include <edm4eic/RawTrackerHit.h>
 #include <edm4eic/TrackerHit.h>
+#include <edm4eic/helix_utils.h>
 #include <edm4hep/EDM4hepVersion.h>
 #include <edm4hep/MCParticleCollection.h>
 #include <edm4hep/SimTrackerHit.h>
@@ -117,6 +118,9 @@ void ActsToTracks::process(const Input& input, const Output& output) const {
       }
       pars.setCovariance(cov);
 
+      edm4eic::Helix h(pars, 1.7);
+      info("helix curvature is {}", h.curvature());
+
       trajectory.addToTrackParameters(pars);
 
       // Fill tracks
EOF
env CXXFLAGS="-I$CMAKE_INSTALL_PREFIX/include -Wno-error=c++11-narrowing" \
    LDFLAGS="-L$CMAKE_INSTALL_PREFIX/lib" \
cmake -S . -B build -DEDM4EIC_ROOT=$CMAKE_INSTALL_PREFIX
cmake --build build -- -j $(nproc)
cmake --install build
popd

source /opt/detector/epic-main/bin/thisepic.sh epic_craterlake_10x100
ddsim --compactFile $DETECTOR_PATH/$DETECTOR_CONFIG.xml --numberOfEvents 1000 --enableGun --gun.thetaMin 'pi/4' --gun.thetaMax '3*pi/4' --gun.distribution uniform --gun.phiMin '0*deg' --gun.phiMax '360*deg' --gun.energy '1*GeV' --gun.particle 'e-' --outputFile sim.edm4hep.root
JANA_PLUGIN_PATH=$CMAKE_INSTALL_PREFIX/lib/EICrecon/plugins LD_LIBRARY_PATH=$PWD/prefix/lib:$LD_LIBRARY_PATH $CMAKE_INSTALL_PREFIX/bin/eicrecon sim.edm4hep.root -Ppodio:output_file=reco.edm4eic.root -Ppodio:output_collections=CentralCKFTracksUnfiltered

This reveals some issues with CMake setup:

  1. Additional helix utility library is redundant. The current the header-only library already exposes the include paths to helix_utils.h, but the translation units from helix_util.cc are only included in the helix_utils lib.
  2. helix_utils is compiled as static by default which will not work as is. Would you consider making functionality header-only? It can be made shared too, but thats a lesser option in some ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants