feat: Add a fastjet plugin #3617
Conversation
github-actions
bot
added
Infrastructure
gagnonlg
force-pushed
the
240916-trackjets
branch
2 times, most recently
from
00e0ff0 to
55c7c22
Compare
gagnonlg
force-pushed
the
240916-trackjets
branch
2 times, most recently
from
99644ce to
880bf6e
Compare
gagnonlg
force-pushed
the
240916-trackjets
branch
from
880bf6e to
6e0a214
Compare
gagnonlg
force-pushed
the
240916-trackjets
branch
from
6e0a214 to
55c9817
Compare
|
We probably need to add this to the CI to make sure it's not instantly broken. |
Right, is there a build in particular in which we turn on plugins? |
|
@gagnonlg At this point, FastJet would have to go into our dependencies build (although we'll likely switch this over to spack some time soon). |
|
Hey, where are we with this one? |
|
|
@asalzburger the fastjet build ob macOS turned out to be a bit tricky. I haven't gotten that to work yet. |
WalkthroughA new option for building the FastJet plugin has been introduced in the Changes
Possibly related PRs
Suggested labels
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
Plugins/FastJet/CMakeLists.txt (1)
1-16: Consider adding version compatibility, young padawan.Version compatibility for the installed targets, specify you should. Future-proof your installation will be.
Add version compatibility to the installation configuration:
install( TARGETS ActsPluginFastJet EXPORT ActsPluginFastJetTargets LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} ) +install( + EXPORT ActsPluginFastJetTargets + FILE ActsPluginFastJetTargets.cmake + NAMESPACE Acts:: + DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/Acts +) +include(CMakePackageConfigHelpers) +write_basic_package_version_file( + ActsPluginFastJetConfigVersion.cmake + VERSION ${PROJECT_VERSION} + COMPATIBILITY SameMajorVersion +)cmake/FindFastJet.cmake (2)
3-9: Paths to success, more hints we need, hmm!Search paths for FastJet, limited they are. Consider adding path hints through CMAKE_PREFIX_PATH or environment variables, you should:
find_library(FastJet_LIBRARY NAMES FastJet fastjet + HINTS + $ENV{FASTJET_ROOT}/lib + $ENV{FASTJET_DIR}/lib DOC "The FastJet library") find_path( FastJet_INCLUDE_DIR fastjet/version.hh + HINTS + $ENV{FASTJET_ROOT}/include + $ENV{FASTJET_DIR}/include DOC "The FastJet include directory" )
27-32: Additional properties, consider you must!For robust linking on all systems, additional properties beneficial they are.
add_library(FastJet SHARED IMPORTED) set_property(TARGET FastJet PROPERTY IMPORTED_LOCATION ${FastJet_LIBRARY}) +get_filename_component(_fastjet_soname "${FastJet_LIBRARY}" NAME) +set_property(TARGET FastJet PROPERTY IMPORTED_SONAME "${_fastjet_soname}") set_property( TARGET FastJet PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${FastJet_INCLUDE_DIR} )Tests/UnitTests/Plugins/FastJet/TrackJetsTests.cpp (3)
42-50: Improve const correctness and error handling, we should.Two improvements, I suggest:
- The size() method, const it should be
- More descriptive error message, provide we must
Apply these changes, you shall:
- std::size_t size() { return m_vec.size(); } + std::size_t size() const { return m_vec.size(); } ConstTrackProxy getTrack(std::size_t i) { if (i < size()) { return m_vec[i]; } - throw std::runtime_error("Too few tracks"); + throw std::out_of_range("Track index " + std::to_string(i) + + " exceeds container size " + std::to_string(size())); }
56-70: Extract magic numbers to named constants, clarity brings this will.Well structured the test is, but magic numbers like 100 and 1e-3, scattered they are.
Improve readability thus:
+ constexpr float TEST_PT = 100.0; + constexpr float TEST_ETA = 0.0; + constexpr float TEST_PHI = 0.0; + constexpr float FLOAT_TOLERANCE = 1e-3; BOOST_AUTO_TEST_CASE(SingleTrack) { TrackContainer tracks; - tracks.insert(Track(100, 0, 0)); + tracks.insert(Track(TEST_PT, TEST_ETA, TEST_PHI)); Acts::FastJet::TrackJetSequence jetSeq = Acts::FastJet::makeTrackJets(tracks); std::vector<fastjet::PseudoJet> jets = jetSeq.jets(); BOOST_CHECK_EQUAL(jets.size(), 1); BOOST_CHECK_EQUAL(jets[0].constituents().size(), 1); BOOST_CHECK_EQUAL(jets[0].constituents()[0].user_index(), 0); - BOOST_CHECK_CLOSE(jets[0].pt(), 100, 1e-3); - BOOST_CHECK_CLOSE(jets[0].eta(), 0, 1e-3); - BOOST_CHECK_CLOSE(jets[0].phi(), 0, 1e-3); + BOOST_CHECK_CLOSE(jets[0].pt(), TEST_PT, FLOAT_TOLERANCE); + BOOST_CHECK_CLOSE(jets[0].eta(), TEST_ETA, FLOAT_TOLERANCE); + BOOST_CHECK_CLOSE(jets[0].phi(), TEST_PHI, FLOAT_TOLERANCE);
113-139: Document the test scenario, understanding brings this will.Good test coverage you have, but documentation of the scenario's purpose, missing it is.
Add comments to explain the test setup:
+ /** + * Test track selection within jet core radius + * Setup: + * - One high-pT track at center (100 GeV) + * - Two tracks within core radius (±0.05) + * - Two tracks outside core radius (±0.2) + * Expected: + * - Only tracks within specified radius (0.1) included + */ BOOST_AUTO_TEST_CASE(TracksInJetCore) {docs/getting_started.md (1)
282-282: Expand the FastJet plugin description, we should.Brief is the current description, hmm. For users to understand the plugin's purpose, more details we need.
Like this, the description should be:
-| ACTS_BUILD_PLUGIN_FASTJET | Build FastJet plugin<br> type: `bool`, default: `OFF` | +| ACTS_BUILD_PLUGIN_FASTJET | Build FastJet plugin for track-jet creation and track iteration within jets<br> type: `bool`, default: `OFF` |Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.ipp (2)
17-29: Simplify the loop, you can, with a range-based for loop.Enhance readability and modernize the code, this change will. Adhere to modern C++ practices, you should.
Apply this diff to refactor the loop:
- for (std::size_t i = 0; i < tracks.size(); i++) { - typename TrackContainer::ConstTrackProxy track = tracks.getTrack(i); - Acts::Vector3 p = track.momentum(); - float m = track.particleHypothesis().mass(); - - float px = p[Acts::eMom0]; - float py = p[Acts::eMom1]; - float pz = p[Acts::eMom2]; - float e = std::sqrt(m * m + px * px + py * py + pz * pz); - - inputs.emplace_back(px, py, pz, e); - inputs.back().set_user_index(i); - } + std::size_t index = 0; + for (const auto& track : tracks) { + Acts::Vector3 p = track.momentum(); + float m = track.particleHypothesis().mass(); + + float px = p[Acts::eMom0]; + float py = p[Acts::eMom1]; + float pz = p[Acts::eMom2]; + float e = std::sqrt(m * m + px * px + py * py + pz * pz); + + inputs.emplace_back(px, py, pz, e); + inputs.back().set_user_index(index++); + }
55-55: As a const reference, declarecst, you should.Modify
cst, you do not. Declaring itconstprevents unintended changes and may optimize performance.Apply this diff to adjust the loop:
- for (fastjet::PseudoJet& cst : sel(jet.constituents())) { + for (const fastjet::PseudoJet& cst : sel(jet.constituents())) {Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.hpp (5)
63-63: Incorrect Doxygen tag, correct it you must.At line 63,
@jetDefshould be@param jetDefto properly document the parameter.Apply this diff to fix the tag:
-/// @jetDef the jet definition to use, defaults to "DefaultJetDefinition" +/// @param jetDef the jet definition to use, defaults to DefaultJetDefinition
62-63: Doxygen comments, consistency seek you should.Ensure all parameters are documented with
@paramtags for clarity.Apply this diff:
/// Create a sequence of track jets /// /// @param tracks the input tracks -/// @jetDef the jet definition to use, defaults to "DefaultJetDefinition" +/// @param jetDef the jet definition to use, defaults to DefaultJetDefinition
27-28: Clarity in parameter description, bring you must.Explain that
etaMaxis an absolute value of pseudorapidity.Apply this diff:
/// @param ptMin the minimum jet pT in GeV -/// @param etaMax the maximum jet absolute eta +/// @param etaMax the maximum absolute jet pseudorapidity (η)
47-49: Unnecessary copy of 'clusterSeq', avoid you should.Pass
clusterSeqby const reference to prevent copying.Apply this diff:
-TrackJetSequence(fastjet::ClusterSequence clusterSeq, +TrackJetSequence(const fastjet::ClusterSequence& clusterSeq, TrackContainer& inputTracks) : m_clusterSeq{std::move(clusterSeq)}, m_inputTracks{inputTracks} {}And remove
std::movesince copying avoided we have:- : m_clusterSeq{std::move(clusterSeq)}, m_inputTracks{inputTracks} {} + : m_clusterSeq{clusterSeq}, m_inputTracks{inputTracks} {}
51-53: Member 'm_clusterSeq' should be a reference, consider you must.To avoid copying when passing by const reference, store a reference.
Apply this diff:
private: - fastjet::ClusterSequence m_clusterSeq; + const fastjet::ClusterSequence& m_clusterSeq; TrackContainer& m_inputTracks;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
CMakeLists.txt(2 hunks)Plugins/CMakeLists.txt(1 hunks)Plugins/FastJet/CMakeLists.txt(1 hunks)Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.hpp(1 hunks)Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.ipp(1 hunks)Tests/UnitTests/Plugins/CMakeLists.txt(1 hunks)Tests/UnitTests/Plugins/FastJet/CMakeLists.txt(1 hunks)Tests/UnitTests/Plugins/FastJet/TrackJetsTests.cpp(1 hunks)cmake/FindFastJet.cmake(1 hunks)docs/getting_started.md(1 hunks)
🔇 Additional comments (18)
Tests/UnitTests/Plugins/FastJet/CMakeLists.txt (2)
2-2: Verify the existence of TrackJetsTests.cpp source file, we shall.
Proper is the unit test registration, but ensure the source file exists, we must.
✅ Verification successful
Found the TrackJetsTests.cpp file is, hmmmm. In the correct location it resides.
In Tests/UnitTests/Plugins/FastJet directory, present the source file is. Correct path alignment with CMakeLists.txt we have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TrackJetsTests.cpp exists
# Expected: Find the source file in the same directory
# Search for the test source file
fd TrackJetsTests.cpp
Length of output: 74
1-1: Verify the ActsPluginFastJet library existence, hmmmm.
Correct the library dependency is, but verify its existence we must.
✅ Verification successful
Correct the library dependency is, young padawan
Properly defined the ActsPluginFastJet library target is, in Plugins/FastJet/CMakeLists.txt. As an INTERFACE library it exists, and correctly linked in the unit tests it is. The Force is strong with this dependency, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for ActsPluginFastJet library definition
# Expected: Find library definition in Plugins/FastJet/CMakeLists.txt
# Search for library definition
fd CMakeLists.txt --exec grep -l "ActsPluginFastJet"
Length of output: 134
Script:
#!/bin/bash
# Search for library target definition in FastJet plugin
cat ./Plugins/FastJet/CMakeLists.txt
Length of output: 523
Plugins/FastJet/CMakeLists.txt (3)
1-2: Wise choice, using INTERFACE library is, hmm.
Correctly declared the library is, for header-only implementation suitable it is.
3-8: Proper include paths configuration, I sense.
Generator expressions for build and install interfaces, properly used they are. Clear separation between build-time and install-time paths, maintained it is.
9-9: Verify the FastJet dependency availability, we must.
Linked correctly the FastJet library is, but verify its presence in different build environments, we should.
✅ Verification successful
Properly configured, the FastJet dependency is, young Padawan
Clear it is, that the Force flows strong through the CMake configuration:
- Controlled by
ACTS_BUILD_PLUGIN_FASTJEToption, the FastJet plugin is - When enabled, properly required through
find_package(FastJet REQUIRED)it becomes - Correctly imported and linked as a shared library, it is
- Clear error messages if not found, the build system provides
No disturbance in the Force I sense. The path to building the FastJet plugin, well-defined it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FastJet dependency configuration in parent CMake files
# Search for FastJet package requirement
rg -l "find_package.*FastJet.*REQUIRED"
# Check for any FastJet-related build options
rg -l "option.*FASTJET"
Length of output: 97
Script:
#!/bin/bash
# Hmmmm, deeper into the CMake files, we must look
# Check the content of root CMakeLists.txt for FastJet configuration
echo "=== Root CMakeLists.txt FastJet Configuration ==="
rg "FastJet" CMakeLists.txt -A 3 -B 3
# Search for FastJet in all CMake files to understand its usage
echo -e "\n=== All FastJet References in CMake Files ==="
fd -e cmake -e txt --exec rg "FastJet" {}
Length of output: 2210
Tests/UnitTests/Plugins/CMakeLists.txt (2)
6-6: Hmmmm, good this change is!
Properly placed, the FastJet plugin inclusion has been. Follow the ways of the CMake Force, it does - consistent with its plugin brethren, it stands. Alphabetical order, maintained it has. Conditional compilation wisdom, strong with this one it is.
6-6: Verify the path to enlightenment, we must!
The build flag's existence, confirm we shall. In the parent CMake files, seek its definition we must.
✅ Verification successful
Hmmmm... Clear the path is, young padawan!
Found the definition in the root CMakeLists.txt, I have. Properly defined as an option with default value OFF, it is. Used consistently across the build system, the flag appears to be. In harmony, the Force is.
Locations where the flag guides the way:
- Root
CMakeLists.txt: Definition as build option - Root
CMakeLists.txt: Conditional check Plugins/CMakeLists.txt: Component additionTests/UnitTests/Plugins/CMakeLists.txt: Test directory inclusion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the FastJet build flag definition
rg "ACTS_BUILD_PLUGIN_FASTJET" --type cmake
Length of output: 347
cmake/FindFastJet.cmake (2)
21-25: Wisdom in these lines, I sense!
Properly handled, the package requirements are. Continue on this path, you may.
34-40: Clear messages and proper handling, strong with the Force this code is!
Well structured the error handling is, and informative the messages are. Proceed with confidence, you may.
Plugins/CMakeLists.txt (2)
6-6: Correct, the placement is! Hmmmm.
In the right section of independent plugins, placed alphabetically between Cuda and FpeMonitoring, this new FastJet plugin is. Follow the established pattern of component declaration, it does.
6-6: Verify the independence of this plugin, we must.
In the independent plugins section, placed this component is. Ensure no hidden dependencies on other ACTS plugins exist, we should.
✅ Verification successful
Independent this plugin is, hmmmm, yes.
Clear as the waters of Dagobah, this verification is. The FastJet plugin:
- Only the FastJet external dependency, it requires
- No other ACTS plugins, it imports
- Interface-only library, it is
- Pure header implementation, maintains it does
- In tests, only core ACTS utilities, it uses
The Force is strong with this one's independence. No hidden dependencies on other ACTS plugins, there are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential dependencies on other ACTS plugins
# Check for includes of other plugins in FastJet plugin headers and sources
echo "Searching for dependencies in FastJet plugin files..."
fd -e hpp -e cpp . | grep -i "fastjet" | xargs rg -l "include.*Plugin"
# Check for references to other plugin names in CMake files
echo "Searching for plugin dependencies in CMake files..."
fd CMakeLists.txt | xargs rg -l "FastJet.*Plugin.*DEPENDS"
Length of output: 412
Script:
#!/bin/bash
# Hmmmm... deeper we must look. Found one test file with Plugin includes, we have.
# Check the specific test file's content, we shall
echo "Content of the test file with Plugin includes:"
cat Tests/UnitTests/Plugins/FastJet/TrackJetsTests.cpp
# The plugin's source files, find we must
echo -e "\nFastJet plugin source files:"
fd -e hpp -e cpp . Plugins/FastJet/
# The CMake configuration, inspect we shall
echo -e "\nCMake configuration for FastJet:"
cat Plugins/FastJet/CMakeLists.txt
Length of output: 5356
CMakeLists.txt (2)
50-50: Approve the option declaration, I do.
Follows the pattern of other plugin options, this does. Wise choice to default to OFF, it is.
377-379: Consider Spack integration, you must.
Mentioned in PR comments, transition to Spack for dependency management is. Prepare for this transition, we should.
Run this command to check if FastJet is already available in Spack:
docs/getting_started.md (1)
282-282: Integration with build examples, verify we must.
If FastJet plugin examples exist, document their build requirements we should. In the examples section, this belongs.
Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.ipp (3)
12-34: Well-implemented, the makeTrackJets function is.
Effectively constructs the track jet sequence, it does. The logic is sound.
36-42: Functionality sound, the jets method is.
Correctly selects jets based on ptMin and etaMax, it does. Implementation is proper.
44-60: Efficiently retrieves tracks in jets, the tracksInJet method does.
Handles optional coreR parameter appropriately. Code is coherent and effective.
Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.hpp (1)
71-71: Definitions in 'TrackJets.ipp', confirm presence we must.
Ensure that all template definitions are in 'TrackJets.ipp' to prevent linker errors.
Run the following script to verify the presence of method definitions:
✅ Verification successful
Present and complete, all template definitions are, hmmmm.
In TrackJets.ipp, found I have:
- Definition of
jets()method, properly templated it is - Definition of
tracksInJet()method, complete it appears - Even bonus definition of
makeTrackJets()function, included it is
Strong with the Force, this implementation file is. No linker errors shall we fear.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'TrackJets.ipp' contains definitions for all methods of 'TrackJetSequence'.
# Test: Search for definitions of 'jets' and 'tracksInJet'. Expect: Definitions found in 'TrackJets.ipp'.
ast-grep --pattern $'template <typename TrackContainer>\nstd::vector<fastjet::PseudoJet> TrackJetSequence<TrackContainer>::jets($_)\n{\n$$$\n}' TrackJets.ipp
ast-grep --pattern $'template <typename TrackContainer>\nstd::vector<typename TrackContainer::TrackProxy> TrackJetSequence<TrackContainer>::tracksInJet($_)\n{\n$$$\n}' TrackJets.ipp
Length of output: 455
Script:
#!/bin/bash
# First, find the correct path to TrackJets.ipp
fd "TrackJets.ipp"
# Let's also check the header file to understand what we're looking for
fd "TrackJets.hpp" --exec cat {}
Length of output: 2481
Script:
#!/bin/bash
# Now that we found the correct path, let's check the implementation file
cat "Plugins/FastJet/include/Acts/Plugins/FastJet/TrackJets.ipp"
Length of output: 2139
| install( | ||
| TARGETS ActsPluginFastJet | ||
| EXPORT ActsPluginFastJetTargets | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
| ) | ||
| install(DIRECTORY include/Acts DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}) |
There was a problem hiding this comment.
💡 Codebase verification
Missing export destination and component specifications, I sense.
Incomplete the installation configuration is, young padawan. Two improvements required are:
- Export configuration destination missing is:
install(EXPORT ActsPluginFastJetTargets DESTINATION ${install_package_config_dir}) - Component specifications absent are, which other plugins use:
COMPONENT dev
🔗 Analysis chain
Installation configuration, complete it appears, but verify component installation we must.
Correctly specified the installation targets are, but ensure proper component installation in the Acts ecosystem, we should.
The Force guides me to verify the complete installation pattern, particularly the export destination and component specifications, hmm.
Hmm, deeper into the installation patterns we must look. The Force tells me to examine how other plugins handle their exports and components, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Acts component installation patterns
# Check other plugins' installation patterns
fd -e txt -x rg "EXPORT.*Targets" {} \;
# Verify include directory structure
fd -t d "include/Acts" -x echo "Found Acts include dir: {}" \;
Length of output: 970
Script:
#!/bin/bash
# Check if FastJet plugin follows the common installation pattern by verifying destination and export commands
rg "DESTINATION.*install_package_config_dir.*ActsPluginFastJetTargets" -A 2 -B 2
# Check include directory structure specifically for FastJet plugin
fd "FastJet" -t d
# Look for component installation patterns in FastJet's CMakeLists
rg "install\(.*COMPONENT" Plugins/FastJet/CMakeLists.txt
Length of output: 255
Script:
#!/bin/bash
# Check common installation patterns in Core and other plugins
rg "install\(EXPORT.*Targets.*DESTINATION" -A 2 -B 2
# Examine FastJet's CMakeLists.txt content
cat Plugins/FastJet/CMakeLists.txt
# Check if there are any component-based installations in other plugins
fd "CMakeLists.txt" Plugins -x rg "COMPONENT" {} \;
Length of output: 1124
| file(READ "${FastJet_INCLUDE_DIR}/fastjet/config_auto.h" FastJet_VERSION_FILE) | ||
| string( | ||
| REGEX MATCH | ||
| "#define FASTJET_PACKAGE_VERSION \"([0-9]+\.[0-9]+\.[0-9]+)\"" | ||
| _ | ||
| ${FastJet_VERSION_FILE} | ||
| ) | ||
|
|
||
| set(FastJet_VERSION ${CMAKE_MATCH_1}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors we must, when version pattern eludes us!
Check if version pattern match successful, you should. Prevent silent failures, this will.
string(
REGEX MATCH
"#define FASTJET_PACKAGE_VERSION \"([0-9]+\.[0-9]+\.[0-9]+)\""
_
${FastJet_VERSION_FILE}
)
+if(NOT CMAKE_MATCH_1)
+ message(FATAL_ERROR "Failed to extract FastJet version from config_auto.h")
+endif()
set(FastJet_VERSION ${CMAKE_MATCH_1})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| file(READ "${FastJet_INCLUDE_DIR}/fastjet/config_auto.h" FastJet_VERSION_FILE) | |
| string( | |
| REGEX MATCH | |
| "#define FASTJET_PACKAGE_VERSION \"([0-9]+\.[0-9]+\.[0-9]+)\"" | |
| _ | |
| ${FastJet_VERSION_FILE} | |
| ) | |
| set(FastJet_VERSION ${CMAKE_MATCH_1}) | |
| file(READ "${FastJet_INCLUDE_DIR}/fastjet/config_auto.h" FastJet_VERSION_FILE) | |
| string( | |
| REGEX MATCH | |
| "#define FASTJET_PACKAGE_VERSION \"([0-9]+\.[0-9]+\.[0-9]+)\"" | |
| _ | |
| ${FastJet_VERSION_FILE} | |
| ) | |
| if(NOT CMAKE_MATCH_1) | |
| message(FATAL_ERROR "Failed to extract FastJet version from config_auto.h") | |
| endif() | |
| set(FastJet_VERSION ${CMAKE_MATCH_1}) |
| trks.end()); | ||
| BOOST_CHECK(std::find(trks.begin(), trks.end(), tracks.getTrack(4)) == | ||
| trks.end()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Additional test cases needed, complete coverage they bring.
Missing test cases, I sense. Add these scenarios, you should:
- Empty track container handling
- Invalid core radius validation
- Performance with many tracks
Example test case to add:
BOOST_AUTO_TEST_CASE(EmptyTrackContainer) {
TrackContainer tracks;
Acts::FastJet::TrackJetSequence jetSeq = Acts::FastJet::makeTrackJets(tracks);
BOOST_CHECK_EQUAL(jetSeq.jets().size(), 0);
}
BOOST_AUTO_TEST_CASE(InvalidCoreRadius) {
TrackContainer tracks;
tracks.insert(Track(100, 0, 0));
Acts::FastJet::TrackJetSequence jetSeq = Acts::FastJet::makeTrackJets(tracks);
BOOST_CHECK_THROW(jetSeq.tracksInJet(jetSeq.jets()[0], -1.0),
std::invalid_argument);
}| Track(float pt, float eta, float phi) { | ||
| m_momentum[0] = pt * std::cos(phi); | ||
| m_momentum[1] = pt * std::sin(phi); | ||
| m_momentum[1] = pt * std::sinh(eta); | ||
| } |
There was a problem hiding this comment.
Fix momentum calculation in Track constructor, a critical bug there is!
Hmmmm, wrong the momentum calculation is. The z-component, missing it is, and the y-component, overwritten it becomes. A typo in the indices, I sense.
Apply this fix, you must:
Track(float pt, float eta, float phi) {
m_momentum[0] = pt * std::cos(phi);
m_momentum[1] = pt * std::sin(phi);
- m_momentum[1] = pt * std::sinh(eta);
+ m_momentum[2] = pt * std::sinh(eta);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Track(float pt, float eta, float phi) { | |
| m_momentum[0] = pt * std::cos(phi); | |
| m_momentum[1] = pt * std::sin(phi); | |
| m_momentum[1] = pt * std::sinh(eta); | |
| } | |
| Track(float pt, float eta, float phi) { | |
| m_momentum[0] = pt * std::cos(phi); | |
| m_momentum[1] = pt * std::sin(phi); | |
| m_momentum[2] = pt * std::sinh(eta); | |
| } |
| if(ACTS_BUILD_PLUGIN_FASTJET) | ||
| find_package(FastJet REQUIRED) | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify minimum FastJet version, you must.
Unlike other dependencies that specify versions, no minimum version for FastJet package, you have defined. Prevent compatibility issues in the future, this will.
Apply this change, you should:
if(ACTS_BUILD_PLUGIN_FASTJET)
- find_package(FastJet REQUIRED)
+ set(_acts_fastjet_version 3.4.0)
+ find_package(FastJet ${_acts_fastjet_version} REQUIRED)
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(ACTS_BUILD_PLUGIN_FASTJET) | |
| find_package(FastJet REQUIRED) | |
| endif() | |
| if(ACTS_BUILD_PLUGIN_FASTJET) | |
| set(_acts_fastjet_version 3.4.0) | |
| find_package(FastJet ${_acts_fastjet_version} REQUIRED) | |
| endif() |
| @@ -279,6 +279,7 @@ components. | |||
| | ACTS_BUILD_PLUGIN_PODIO | Build Podio plugin<br> type: `bool`, default: `OFF` | | |||
| | ACTS_BUILD_PLUGIN_EDM4HEP | Build EDM4hep plugin<br> type: `bool`, default: `OFF` | | |||
| | ACTS_BUILD_PLUGIN_FPEMON | Build FPE monitoring plugin<br> type: `bool`, default: `OFF` | | |||
| | ACTS_BUILD_PLUGIN_FASTJET | Build FastJet plugin<br> type: `bool`, default: `OFF` | | |||
There was a problem hiding this comment.
Missing prerequisites for FastJet plugin, I sense.
Document the FastJet dependency requirements and minimum version, we must. In the prerequisites section, this information belongs.
Add this diff to the optional dependencies section, you should:
+ [FastJet](http://fastjet.fr/) for the FastJet pluginCommittable suggestion skipped: line range outside the PR's diff.
This PR adds a fastjet plugin which allows creating track-jets, as well as iterating over tracks in the core of such jets, subject to kinematic cuts.
Such a plugin would be useful, for example, to studies on using jet information as a track-density proxy before or during ambiguity solving.
This raises the question of whether or not this belongs in the scope of a tracking library, but I think it does since this can be useful e.g. in between track finding & ambiguity solving (or during ambi-solving)
Action items
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests