Fixes for maps.#4175
Merged
osbornjd merged 9 commits intosPHENIX-Collaboration:masterfrom Feb 17, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTpcClusterizer: adds an m_is_data flag and mc_sectors mapping; refactors channel-masking input to read paired fields ( Changes
Sequence Diagram(s)Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/tpc/TpcClusterizer.cc (1)
1875-1903:⚠️ Potential issue | 🟠 MajorAdd missing validation for
LayerandSidefrom CDB data — external calibration bounds must be checked.The bounds check on
Sec(lines 1884–1890) correctly validates external calibration data. However,LayerandSideare also sourced from the CDB without validation. TPC layers must fall within [7, 48] (confirmed by validation patterns inTpcRawWriter.cc), andSidemust be 0 or 1. If the CDB contains an unexpected value,TpcDefs::genHitSetKey(Layer, Sector, Side)at line 1901 will produce a hitsetkey that never matches any actual TPC hitset, silently causing the mask entry to be ignored with no diagnostic output.Proposed fix
if (Sec < 0 || Sec >= 12) { std::cout << PHWHERE << "WARNING: sector index " << Sec << " out of range [0,11] in " << dbName << ", skipping channel " << i << std::endl; continue; } int Layer = (m_isSimulation) ? Layer0 : Layer1; int Pad = (m_isSimulation) ? Pad0 : Pad1; int Sector = (m_isSimulation) ? mc_sectors[Sec] : Sec; + if (Layer < 7 || Layer > 48) + { + std::cout << PHWHERE << "WARNING: layer " << Layer + << " out of TPC range [7,48] in " << dbName + << ", skipping channel " << i << std::endl; + continue; + } + + if (Side < 0 || Side > 1) + { + std::cout << PHWHERE << "WARNING: side " << Side + << " out of range [0,1] in " << dbName + << ", skipping channel " << i << std::endl; + continue; + } + if (Verbosity() > VERBOSITY_A_LOT)
added 2 commits
February 11, 2026 22:25
Build & test reportReport for commit 3fd2a487f9877573ef098986a781258ab695ae7b:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit edb448dc40e7f45f05790f4baddd6c2c779a39be:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 0bc9fe491953708645e1909132a86570ebad9d9d:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 2305b2220986f5576c5fcd7782d2e79a0c7bd1b7:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
requested changes
Feb 12, 2026
Contributor
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
There is a flag at the macro level that is set based on the CDB tag to determine whether or not the reconstruction is being run on data or simulation
https://github.com/sPHENIX-Collaboration/macros/blob/b8edb9fd697616a3250dabd2608ced54b446ed89/common/GlobalVariables.C#L136
Let's use that so that all modules are consistent
osbornjd
reviewed
Feb 12, 2026
|
|
||
| if (Sec < 0 || Sec >= 12) | ||
| { | ||
| std::cout << PHWHERE << "WARNING: sector index " << Sec |
Contributor
There was a problem hiding this comment.
Can we wrap these into Verbosity() statements?
osbornjd
reviewed
Feb 12, 2026
|
|
||
| int m_runNumber = -1; // Store run number from Event Header | ||
| bool m_isSimulation = true; // Default true; Updated based on run number | ||
| int mc_sectors[12]{5, 4, 3, 2, 1, 0, 11, 10, 9, 8, 7, 6}; |
Contributor
There was a problem hiding this comment.
I am nervous about having this hard coded. If it ever changes, this will silently break functionality. Why is the sector numbering scheme in the CDB object different for data and sim?
Contributor
Author
There was a problem hiding this comment.
I have removed runNumber and m_isSimulation.
The reason there is different numbering scheme in data and sim is because that is how TPC is. I made a map which can work for both data and sim. And the numbering scheme is therefore have to be fixed carefully. Now Is it ok?
Contributor
There was a problem hiding this comment.
I don't understand because the sector and layer should be coming from the same object, the hitsetkey (regardless of data or sim) , e.g. here. For a given sector, side, and layer the same hitsetkey should be generated. If there is some difference between data and simulation that implies there is a problem in the numbering scheme the hit unpacker is providing. Can you make a slide that shows the numbering scheme for data and simulation as you are finding them here? I'm thinking of a plot for data and sim where you show the phi distribution of hits or clusters, and where the sectors are for each. You could also check that for the same hitsetkey in either data or sim you get different phi regions - that would indicate a problem upstream from the clusterizer that should be fixed
Build & test reportReport for commit b42fdf57f3efbd78048290d274d421ac2e8478ca:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 89772f9056642c741f192da7953b23a5002c3df1:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4a18fc36a8b4747579510b222890db26799e2a96:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit f03fde5c4c04238a0c4d157abe7ec30dfc10a669:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4c4fdf492349cc2d3b8fd16219d98ffde2260131:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
approved these changes
Feb 17, 2026
Contributor
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
Looks good, the clang-tidy errors are benign
osbornjd
merged commit 0fdd99a
into
sPHENIX-Collaboration:master
21 of 22 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Summary
This PR adds dual-mode channel masking in the TPC clusterizer to support both simulation and data channel-map formats and includes input validation for masked-channel entries.
Motivation / context
Simulation and data channel maps use different layer/pad encodings and sector conventions. Without explicit mode selection and remapping, masks from one domain can be mis-applied to the other, degrading reconstruction. This change provides an explicit data/simulation mode and remaps sectors for simulation input.
Key changes
SetIsData(bool)to select data (true) or simulation (false) mode.bool m_is_data.int mc_sectors[12] = {5,4,3,2,1,0,11,10,9,8,7,6}used for sector remapping in simulation mode.m_is_data == false(simulation): use layer0/pad0 and map sector viamc_sectors[Sec].m_is_data == true(data): use layer1/pad1 and use sector as provided.Potential risk areas
Possible future improvements
AI note: This summary was produced with AI assistance. Please verify the exact method name, default mode behavior, and that the hard-coded sector mapping matches detector conventions before merging.