Upgrade to WireTracker_info#83
Conversation
|
Thanks for this PR! |
|
Hi! When I run the Do I need to do anything else before running the fitter with |
@sfranchel , it seems you missed to change the extension type in your k4geo PR. Could you update there? |
|
Ah sorry, nevermind. The new |
|
Thank you @andread3vita and @s6anloes for looking into this, I'll try replicate the error on my side and investigate the issue. Will report here updates. |
s6anloes
left a comment
There was a problem hiding this comment.
I have investigated the failing GenFit test a bit and have found a solution.
I will comment all the files that need to be changed, also in key4hep/k4geo#581 .
TODO: check also if digitiser is impacted
Many thanks @s6anloes for pinning down the issue! Based on your suggestions I have updated the way that the DCH_info pointers are retrieved, and it seems to be working now. All ctests are passing locally! 🥳 |
|
Hi @sfranchel! I confirm that this PR does not affect the fitter. It looks good to me! |
SanghyunKo
left a comment
There was a problem hiding this comment.
I have one small comment on the Vector3D typedef, and another major request on catching the section cellID encoding scheme.
SanghyunKo
left a comment
There was a problem hiding this comment.
m_dc_decoder can be nullptr when the tracker is not a wire tracker:
k4RecTracker/Tracking/components/GenfitTrackFitter.cpp
Lines 155 to 187 in c701eca
(not a practical use-case, but safer to guard from nullptr since it is allowed in principle)
|
Now looks good to me :) |
Ensure that polymorphism work with WireTracker_info subclasses Co-authored-by: Andreas Loeschcke Centeno <53434935+s6anloes@users.noreply.github.com>
* use a try catch for retrival of sector * get the dch pointer via dinamic cast of the WireTracker_info_struct * fix v1 of the dch digitizer * update some comments and function naming
Co-authored-by: Sanghyun Ko <37464245+SanghyunKo@users.noreply.github.com>
bd84e2a to
894a381
Compare
|
It seems the tests are failing because the @jmcarcell , do you have experience with this problem? |
|
Hi @s6anloes that |
|
Okay so now the tests fail for alma9 and ubuntu26, but run for ubuntu24. For ubuntu24: For alma9: Could it be a problem with the new lcg stacks of key4hep, @jmcarcell @andresailer ? $ echo $K4GEO
/cvmfs/sft-nightlies.cern.ch/lcg/nightlies/devkey-head/Sat/k4geo/HEAD/x86_64-el9-gcc14-opt/share/k4geo
$ ls /cvmfs/sft-nightlies.cern.ch/lcg/nightlies/devkey-head/Sat/k4geo/HEAD/x86_64-el9-gcc14-opt/include/detectorCommon/
DetUtils_k4geo.h WireTracker_info.h xtalk_neighbors_moduleThetaMergedSegmentation.h |
Ignore this for now, may be related to https://gitlab.cern.ch/gaudi/Gaudi/-/work_items/398. |
What exactly do you mean with 'this'? Or do you mean we should ignore the failing test, because it should run locally, meaning we can go ahead and merge? Or do we ignore in the sense that we have to wait until it is fixed on the Gaudi side? |
|
That you can merge, if it's that issue in Gaudi then once this PR lands in the builds then CI should work fine. |
|
I don't have permission to merge until the checks succeed it appears. |
BEGINRELEASENOTES
ENDRELEASENOTES
Updating DCH digitizers and part of the tracking code to use the
WireTracker_infostructure allowing to share more easily code between Drift Chamber and Straw Tubes tracker.This PR will need first the changes in k4geo to be merged:
key4hep/k4geo#581
ccing @s6anloes