-
Notifications
You must be signed in to change notification settings - Fork 41
Add timeframe splitting framework to EICrecon #2824
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.
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
base: main
Are you sure you want to change the base?
Changes from 30 commits
409dfe3
1d2f7fc
9a01c5b
b73d352
1437f6f
ccbd1a8
a711130
7311db9
c828d72
49fee5e
0e839da
af6d50f
c90ea2c
4520646
2a3564a
6fcd19e
d2ce953
610d2fa
c1f5fc7
a5fd275
4e373f9
8c5ca6e
e2695c7
ebe13a5
72a8245
a8ecb29
ca90e3e
04428f0
67292c3
9bdd943
9d38152
9427b86
47582e6
16ca933
34e9d69
d7c313d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1321,6 +1321,94 @@ jobs: | |
| if-no-files-found: error | ||
| include-hidden-files: true | ||
|
|
||
| eicrecon-timeframe-splitting: | ||
| runs-on: ubuntu-24.04 | ||
| needs: | ||
| - build | ||
| - npsim-dis | ||
| - npsim-minbias | ||
| strategy: | ||
| matrix: | ||
| include: | ||
| - CXX: clang++ | ||
| beam: 18x275 | ||
| minq2: 100 | ||
| detector_config: craterlake_18x275 | ||
| sanitizer: ASAN | ||
|
|
||
| - CXX: clang++ | ||
| beam: 18x275 | ||
| minq2: 100 | ||
| detector_config: craterlake_18x275 | ||
|
|
||
| sanitizer: TSAN | ||
| nthreads: 4 | ||
|
|
||
| steps: | ||
| - name: Checkout .github and scripts | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| sparse-checkout: | | ||
| .github | ||
| src/scripts | ||
|
|
||
| - name: Download install directory | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: install-${{ matrix.CXX }}-eic-shell-Release-${{ env.platform }}-${{ env.release }}-${{ matrix.sanitizer }} | ||
|
|
||
| - name: Unarchive install directory | ||
| run: tar -xaf install.tar.zst | ||
|
|
||
| - name: Download simulation input | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: sim_dis_${{ matrix.beam }}_minQ2=${{ matrix.minq2 }}_${{ matrix.detector_config }}.edm4hep.rnt.root | ||
|
|
||
| - name: Setup cvmfs | ||
| uses: cvmfs-contrib/github-action-cvmfs@v5 | ||
|
|
||
| - name: Run EICrecon with timeframe splitting | ||
| uses: eic/run-cvmfs-osg-eic-shell@main | ||
| with: | ||
| organization: "${{ env.organization }}" | ||
| platform-release: "${{ env.platform }}:${{ env.release }}" | ||
| setup: "/opt/detector/epic-${{ env.detector-version }}/bin/thisepic.sh" | ||
| run: | | ||
| echo "::add-matcher::${{github.workspace}}/.github/ubsan.json" | ||
| echo "::add-matcher::${{github.workspace}}/.github/eicrecon.json" | ||
|
|
||
| export DETECTOR_CONFIG=${DETECTOR}_${{ matrix.detector_config }} | ||
| export LD_LIBRARY_PATH=$PWD/install/lib:$LD_LIBRARY_PATH | ||
| export JANA_PLUGIN_PATH=$PWD/install/lib/EICrecon/plugins:/usr/local/plugins | ||
|
|
||
| prmon --json-summary timeframe_splitter_${{matrix.sanitizer}}.prmon.json -- \ | ||
| $PWD/install/bin/eicrecon \ | ||
| ${{env.JANA_OPTIONS}} \ | ||
| ${{matrix.nthreads > 1 && format('-Pnthreads={0}', matrix.nthreads) || ''}} \ | ||
| -Ppodio:output_file=timeframe_splitter_${{matrix.sanitizer}}.edm4eic.root \ | ||
| sim_dis_${{matrix.beam}}_minQ2=${{matrix.minq2}}_${{ matrix.detector_config }}.edm4hep.rnt.root \ | ||
| -Psplit_timeframes=true | ||
|
|
||
| - name: Verify output file | ||
| run: | | ||
| test -s timeframe_splitter_${{matrix.sanitizer}}.edm4eic.root | ||
|
|
||
| - name: Upload output | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: timeframe_splitter_${{matrix.sanitizer}}.edm4eic.root | ||
| path: timeframe_splitter_${{matrix.sanitizer}}.edm4eic.root | ||
| if-no-files-found: error | ||
|
|
||
| - name: Upload prmon report | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: timeframe_splitter_${{matrix.sanitizer}}.prmon.json | ||
| path: timeframe_splitter_${{matrix.sanitizer}}.prmon.json | ||
| if-no-files-found: error | ||
|
|
||
|
|
||
| compare-single-multi-threaded: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the comparison between single and multi-threaded we should also compare the single and multi-threaded timeframe splitter execution branches. And we should compare timeframe splitting analysis with the regular physics event reconstruction on the same physics simulation input (single-threaded for now). |
||
| runs-on: ubuntu-24.04 | ||
| needs: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| set(PLUGIN_NAME "algorithms_event_building") | ||
|
|
||
| # Function creates ${PLUGIN_NAME}_plugin and ${PLUGIN_NAME}_library targets | ||
| # Setting default includes, libraries and installation paths | ||
| plugin_headers_only(${PLUGIN_NAME}) | ||
|
|
||
| # Find dependencies | ||
| plugin_add_algorithms(${PLUGIN_NAME}) |
|
ruse-traveler marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| // SPDX-License-Identifier: LGPL-3.0-or-later | ||
| // Copyright (C) 2026 Takuya Kumaoka | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <algorithm> | ||
| #include <cmath> | ||
| #include <string_view> | ||
| #include <vector> | ||
|
|
||
| #include <algorithms/algorithm.h> | ||
|
|
||
| #include "algorithms/interfaces/WithPodConfig.h" | ||
| #include "algorithms/event_building/HitTimeAlignmentConfig.h" | ||
|
|
||
| namespace eicrecon { | ||
|
|
||
| template <typename HitT> | ||
| using HitTimeAlignmentAlgorithm = | ||
| algorithms::Algorithm<algorithms::Input<const typename HitT::collection_type>, | ||
| algorithms::Output<typename HitT::collection_type>>; | ||
|
|
||
| template <typename HitT> | ||
| class HitTimeAlignment : public HitTimeAlignmentAlgorithm<HitT>, | ||
| public WithPodConfig<HitTimeAlignmentConfig> { | ||
| public: | ||
| using AlgorithmT = HitTimeAlignmentAlgorithm<HitT>; | ||
|
|
||
| explicit HitTimeAlignment(std::string_view name) | ||
| : AlgorithmT{name, | ||
| {"inputHits"}, | ||
| {"outputHits"}, | ||
| "Correct reconstructed hit times for propagation and sort by time."} {} | ||
|
|
||
| void init() final {} | ||
|
|
||
| void process(const typename AlgorithmT::Input& input, | ||
| const typename AlgorithmT::Output& output) const final { | ||
| const auto [hits_in] = input; | ||
| auto [hits_out] = output; | ||
|
|
||
| std::vector<typename HitT::mutable_type> sorted_hits; | ||
| sorted_hits.reserve(hits_in->size()); | ||
| for (const auto& hit : *hits_in) { | ||
| auto copied_hit = hit.clone(); | ||
| const auto position = hit.getPosition(); | ||
| const auto radius = std::sqrt(position[0] * position[0] + position[1] * position[1] + | ||
| position[2] * position[2]); | ||
| const auto average_time_of_flight = radius * this->m_cfg.reference_inverse_velocity; | ||
| copied_hit.setTime(hit.getTime() - average_time_of_flight); | ||
| sorted_hits.push_back(copied_hit); | ||
| } | ||
|
|
||
| std::stable_sort(sorted_hits.begin(), sorted_hits.end(), [](const auto& lhs, const auto& rhs) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just in case someone has the same thought, the size of a hit here is typically < 64 bytes so a hit fits in an L1 cache line and so there is no benefit to sort an iota to avoid the move costs. Microbenchmark could be useful but not necessary since likely not a major impact on performance here at this point. |
||
| return lhs.getTime() < rhs.getTime(); | ||
| }); | ||
|
|
||
| for (const auto& hit : sorted_hits) { | ||
| hits_out->push_back(hit); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| } // namespace eicrecon | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // SPDX-License-Identifier: LGPL-3.0-or-later | ||
| // Copyright (C) 2026 Takuya Kumaoka | ||
|
|
||
| #pragma once | ||
|
|
||
| namespace eicrecon { | ||
|
|
||
| struct HitTimeAlignmentConfig { | ||
| double reference_inverse_velocity = | ||
| 0.0034; //< ns/mm estimated by MC average time of flight / distance from IP to calorimeter | ||
| }; | ||
|
|
||
| } // namespace eicrecon |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
| // Copyright (C) 2022 - 2025 Whitney Armstrong, Sylvester Joosten, Chao Peng, David Lawrence, Wouter Deconinck, Kolja Kauder, Nathan Brei, Dmitry Kalinkin, Derek Anderson, Michael Pitt | ||
|
|
||
| #include <Evaluator/DD4hepUnits.h> | ||
| #include <JANA/JApplication.h> | ||
| #include <JANA/JApplicationFwd.h> | ||
| #include <JANA/Utils/JEventLevel.h> | ||
| #include <JANA/Utils/JTypeInfo.h> | ||
| #include <cmath> | ||
| #include <string> | ||
|
|
@@ -23,6 +25,9 @@ void InitPlugin(JApplication* app) { | |
| using namespace eicrecon; | ||
|
|
||
| InitJANAPlugin(app); | ||
| const bool split_timeframes = | ||
| app->RegisterParameter<bool>("split_timeframes", false, "Enable timeframe splitting"); | ||
| const auto hit_level = split_timeframes ? JEventLevel::Timeslice : JEventLevel::PhysicsEvent; | ||
|
|
||
| app->Add(new JOmniFactoryGeneratorT<CalorimeterHitDigi_factory>( | ||
| "B0ECalRawHits", {"EventHeader", "B0ECalHits"}, | ||
|
|
@@ -40,7 +45,7 @@ void InitPlugin(JApplication* app) { | |
| .corrMeanScale = "1.0", | ||
| .readout = "B0ECalHits", | ||
| }, | ||
| app)); | ||
| app, hit_level)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand semantically why the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. The main reason I attached hit_level to the factories is that time splitting needs to be optional. Whether time splitting is enabled or disabled, the factories other than the time-splitting component itself should behave in the same way, without needing to be aware of whether their input collections have gone through time splitting. The time-splitting mechanism is intended to intercept the normal data flow, so I have also tried to keep the collection names unchanged between the normal and time-splitting workflows. If we instead use different collection names for the time-splitting workflow, we would effectively need a separate version of almost every detector plugin under src/detectors/ with nearly identical configuration except for the collection names. We used this approach previously, but it made maintenance difficult because changes to the original detector plugins also had to be manually propagated to the time-splitting versions. With Nathan’s recent updates, it became possible to avoid this duplication and use the current approach. My intention with hit_level was therefore to allow the same factories and collection names to be used in both workflows, while changing only the level from which the collections are retrieved. Sorry, I may not be able to explain the JANA2-level architecture well enough by myself. I am currently visiting JLab and working with Nathan, so if you have some time, I would appreciate the opportunity to discuss this with you and Nathan together. In parallel, we will continue to optimize and improve this factory. However, we would also like to get this PR merged before the next week's streaming reconstruction workshop, if possible.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you misunderstood my suggestion. I'm not suggesting you change the names of collections, but use the level at which a collection is defined as something programmatically attached to the collection name.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Inside JANA2, factories are always attached to an EventLevel. It's just implicit when all of your factories are PhysicsEvent-level.
That may be so in this case, but JANA2 emphatically does not make that assumption globally. Collections at different levels are not substitutable because they have different semantics from the physicist's perspective, and factories are lazily-evaluated collections. If a collection is missing, JANA2 won't "search upwards" among its parents, and analogously, it won't execute a factory based off of collection name alone. Take a look at how JANA2 models factories and collections internally:
There is exactly one parameter,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is indeed exactly one parameter, it should be defined once, in one place. |
||
| app->Add(new JOmniFactoryGeneratorT<CalorimeterHitReco_factory>( | ||
| "B0ECalRecHits", {"B0ECalRawHits"}, {"B0ECalRecHits"}, | ||
| { | ||
|
|
@@ -55,7 +60,7 @@ void InitPlugin(JApplication* app) { | |
| .readout = "B0ECalHits", | ||
| .sectorField = "sector", | ||
| }, | ||
| app)); | ||
| app, hit_level)); | ||
| app->Add(new JOmniFactoryGeneratorT<CalorimeterTruthClustering_factory>( | ||
| "B0ECalTruthProtoClusters", {"B0ECalRecHits", "B0ECalHits"}, {"B0ECalTruthProtoClusters"}, | ||
| app)); | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?