Another try.#4287
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR standardizes tracker cluster fields to fixed-width integer types and introduces TrkrClusterv6. The base TrkrCluster class gains updated sentinel constants and many convenience accessor stubs. Existing versions v1–v5 narrow ADC from unsigned int to uint16_t and size fields from char to uint8_t. The new v6 subclass stores 2D local position and provides lifecycle, deep-copy, and validity semantics, with deprecated global-style accessors. ChangesTracker cluster type modernization and TrkrClusterv6 introduction
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)offline/packages/trackbase/TrkrClusterv1.ccIn file included from offline/packages/trackbase/TrkrClusterv1.cc:7: ... [truncated 1252 characters] ... /include" offline/packages/trackbase/TrkrClusterv2.ccIn file included from offline/packages/trackbase/TrkrClusterv2.cc:7: ... [truncated 1252 characters] ... /include" offline/packages/trackbase/TrkrClusterv3.ccIn file included from offline/packages/trackbase/TrkrClusterv3.cc:7: ... [truncated 1252 characters] ... /include" 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}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f033c91c-adfb-4e8d-aa42-00e6dd7b3d04
📒 Files selected for processing (5)
offline/packages/trackbase/Makefile.amoffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv6.ccoffline/packages/trackbase/TrkrClusterv6.hoffline/packages/trackbase/TrkrClusterv6LinkDef.h
Build & test reportReport for commit 50caed7b44fe0936ab8c7fa38a10107e542c5d27:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 771f1964b313956f323e3de2671edaf8a94569f6:
Automatically generated by sPHENIX Jenkins continuous integration |
|
So when will be added? |
| #ifndef TRACKBASE_TRKRCLUSTERV6_H | ||
| #define TRACKBASE_TRKRCLUSTERV6_H | ||
|
|
||
| #include <iostream> |
There was a problem hiding this comment.
The include file ordering is really important:
https://wiki.sphenix.bnl.gov/index.php?title=Codingconventions#Include_Files
I see other sources here having the same problem - don't worry about that - we'll deal with those later.
| unsigned int getAdc() const override { return m_adc; } | ||
| void setAdc(unsigned int adc) override { m_adc = adc; } | ||
|
|
||
| unsigned int getMaxAdc() const override { return m_maxadc; } |
There was a problem hiding this comment.
Is it unsigned int or uint16_t? Please make this consistent. Likely m_adc should be the same data type
| float getX() const override | ||
| { | ||
| std::cout << "Deprecated getx trkrcluster function!" << std::endl; | ||
| return NAN; |
There was a problem hiding this comment.
use td::numeric_limits::quiet_NaN() instead of NAN
| float m_padmax; | ||
| float m_tbinmax; | ||
| unsigned char m_rsize; // 8bit | ||
| char m_phisize; // 8bit |
There was a problem hiding this comment.
A lot of the char's look like unsigned int's and all are initialized to 0, not to some invalid negative value. , So they might better be unsigned chars
| char m_srmix; // 8bit | ||
| char m_tlmix; // 8bit | ||
| char m_trmix; // 8bit | ||
| float m_phibinlo; |
There was a problem hiding this comment.
bin sounds like it should be an integer data type
| float m_local[2]{std::numeric_limits<float>::quiet_NaN(), std::numeric_limits<float>::quiet_NaN()}; | ||
| //< 2D local position [cm] 2 * 32 64bit - cumul 1*64 | ||
| TrkrDefs::subsurfkey m_subsurfkey; //< unique identifier for hitsetkey-surface maps 16 bit | ||
| float m_phierr; |
There was a problem hiding this comment.
move the long list of initializers from the .cc to here
There was a problem hiding this comment.
remove the binary, it must not go into our repo
| return 0; | ||
| } | ||
| } | ||
| if (m_adc == 0xFFFF) |
There was a problem hiding this comment.
use std::numeric_limits::max() instead of 0xFFFF
| } | ||
| } // namespace | ||
|
|
||
| TrkrClusterv6::TrkrClusterv6() |
There was a problem hiding this comment.
once you move the initializers to the .h file, the ctor is empty and can be declared default in the .h file
|
This is more of a general comment for this package - the v6 contains a lot of implementations of seemingly obsolete methods. If they are are really obsolete the setters should be removed from the virtual base class. this will leave any readback of old versions functional but it will stop the propagation of the use of obsolete set methods which do not have any effect |
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42f2d772-1f05-45af-b5f0-5df62cd980f6
📒 Files selected for processing (12)
offline/packages/TrackingDiagnostics/.TrackResiduals.cc.swooffline/packages/TrackingDiagnostics/.TrackResiduals.h.swooffline/packages/TrackingDiagnostics/.TrkrNtuplizer.h.swooffline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/TrackingDiagnostics/TrkrNtuplizer.ccoffline/packages/tpc/.TpcClusterizer.cc.swooffline/packages/tpc/TpcClusterizer.ccoffline/packages/trackbase/.TrkrClusterv6.h.swooffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv6.ccoffline/packages/trackbase/TrkrClusterv6.h
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/trackbase/TrkrClusterv6.h
| virtual void setAdc(uint16_t) {} | ||
| virtual uint16_t getAdc() const { return UINT16_MAX; } | ||
| virtual void setMaxAdc(uint16_t) {} | ||
| virtual unsigned int getMaxAdc() const { return UINT_MAX; } | ||
| virtual char getOverlap() const { return std::numeric_limits<char>::max(); } | ||
| virtual uint16_t getMaxAdc() const { return UINT16_MAX; } |
There was a problem hiding this comment.
Narrowing total cluster ADC to 16 bits will truncate valid TPC charge.
TpcClusterizer::calc_cluster_parameter accumulates adc_sum across the full cluster and passes it into setAdc(...). That total can exceed 65535 even when each individual sample fits in 16 bits, so this API change silently wraps real charge and can even collide with the UINT16_MAX invalid sentinel used by TrkrClusterv6::isValid(). The total cluster ADC needs to stay wider than the per-bin ADC fields.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab9494a8-fc6c-41f1-988c-17c64c364433
📒 Files selected for processing (9)
offline/packages/trackbase/.TrkrClusterv6.h.swnoffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv1.hoffline/packages/trackbase/TrkrClusterv2.hoffline/packages/trackbase/TrkrClusterv3.hoffline/packages/trackbase/TrkrClusterv4.hoffline/packages/trackbase/TrkrClusterv5.hoffline/packages/trackbase/TrkrClusterv6.ccoffline/packages/trackbase/TrkrClusterv6.h
💤 Files with no reviewable changes (1)
- offline/packages/trackbase/TrkrClusterv6.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- offline/packages/trackbase/TrkrCluster.h
- offline/packages/trackbase/TrkrClusterv6.h
Build & test reportReport for commit 5f6ac3ba02dbfec1107750fb59ec8b9e19fb66fa:
Automatically generated by sPHENIX Jenkins continuous integration |
|
I hope this is good! |
|
I fear this is completely broken now. We cannot change any data members of existing classes, these changes will render existing DSTs unreadable. I would suggest to preserver the v6 and start a fresh PR with those. We probably should discuss this in person first - I see a few ways to do this but it depends on what you actually need. Also your environment for some reason adds hidden files like .TpcClusterizer.cc.swo to the PR - you really need to change that. |
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
I would agree with Chris, this will break all of our DST readback. Perhaps we should chat about what you are trying to do - at the very least we should just start with only the v6 version and getting that in
3 participants
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)
TrkrClusterv6 and TPC cluster debugging additions
Motivation / context
Add a richer cluster representation (v6) to support TPC debugging and detailed offline diagnostics, and standardize cluster field types across existing versions (v1–v5) for tighter, explicit widths and consistent sentinel handling.
Key changes
Potential risk areas
Possible future improvements