Fwd Tracking upgrades for Run22 productions #731
Conversation
jdbrice
force-pushed
the
FwdTrackingForRun22
branch
from
39e63ee to
d578543
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the Forward Tracking upgrades for Run22 productions by reducing debug logging and improving memory consumption. It removes or wraps debug logging in constexpr flags to eliminate them from production running.
Key changes:
- Wrapping debug output in compile-time constants to remove it from production builds
- Memory consumption improvements and optimizations
- Refactoring track fitting workflow with better organization and additional track types
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TrackFitter.h | Major refactoring with debug logging wrapped in constexpr flags, new track fitting methods, and memory optimizations |
| ObjExporter.h | Disables verbose output by setting flag to false |
| GenfitTrackResult.h | New file containing extracted classes for track results and event statistics |
| FwdTracker.h | Extensive refactoring of track fitting workflow with new track types and better organization |
| FwdHit.h | Code optimizations with default constructors and improved data structures |
| FwdGeomUtils.h | New geometry utility methods for FST sensors and FTT quadrants |
| FwdDataSource.h | Adds EPD hits support |
| FitterUtils.h | Improvements to seed fitting algorithms and debug output |
| StFwdTrackMaker.h | Interface improvements and hit loader refactoring |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| static const int sNFstLayers = 3; | ||
| FwdSystem(const int ndisks = FwdSystem::sNFwdLayers) : KiTrack::ISectorSystem(), mNDisks(ndisks){}; | ||
| ~FwdSystem(){/* */}; | ||
| ~FwdSystem() = default; |
There was a problem hiding this comment.
Passing all tests suggests that this works with the current compilers. Interesting. When I tried to use it last time compilations failed.
|
Follow @genevb comment on FWD PicoDst to wrap all LOG calls in a |
|
|
||
| #include <vector> | ||
| #include <memory> | ||
| #include "malloc.h" |
There was a problem hiding this comment.
Use <cstdlib> instead of malloc.h for better C++ standards compliance. The malloc.h header is not portable and is deprecated.
| #include "malloc.h" | |
| #include <cstdlib> |
|
|
||
| // these are the sector ids mapped to layers | ||
| int map[] = {0, 0, 0, 0, 0, 1, 2, 0, 0, 3, 4, 5, 6}; // ftsref6a | ||
| static const std::array<int,14> sector_map = {0, 0, 0, 0, 0, 1, 2, 0, 0, 3, 4, 5, 6, 7}; // ftsref6a |
There was a problem hiding this comment.
[nitpick] Magic numbers in the array should be documented or replaced with named constants. Consider adding comments explaining what each index represents.
| // this disables logging at compile time | ||
| constexpr static int kLogVerbose = 10; | ||
| constexpr static int kLogInfo = 1; | ||
| constexpr static int kLogSilent = 0; | ||
| constexpr static int kLogLevel = kLogSilent; |
There was a problem hiding this comment.
[nitpick] Consider using an enum class instead of separate integer constants for better type safety and to avoid potential value conflicts.
| // this disables logging at compile time | |
| constexpr static int kLogVerbose = 10; | |
| constexpr static int kLogInfo = 1; | |
| constexpr static int kLogSilent = 0; | |
| constexpr static int kLogLevel = kLogSilent; | |
| // Use enum class for log levels for better type safety | |
| enum class LogLevel : int { | |
| Verbose = 10, | |
| Info = 1, | |
| Silent = 0 | |
| }; | |
| constexpr static LogLevel kLogLevel = LogLevel::Silent; |
plexoos
left a comment
plexoos
left a comment
There was a problem hiding this comment.
This PR is a mix of many unrelated changes, which makes it hard to follow the logic. I went through the commits as best I could—aside from a few minor issues that aren’t worth blocking over, I’m giving up.
Since there’s an urgent need to get some of this into the next release and into production, we’ll merge it as is.
Co-authored-by: Copilot <[email protected]>
4 participants
This PR removes or wraps debug logging etc. in constexpr flags to remove them from production running.
Various improvements for memory consumption.