HGTD_GeoModel improvements and cleanup
- Moving to
ATH_MSG_XYZmacros for messages as requested in !39931 (merged) - Removing confusing printout of unused db parameters (z positions of cylindrical volumes) as pointed out by @menke in ATLSWUPGR-119
- Removing hardcoded overrides of db values that were anyway identical to what's in the db (peripheral electronics)
- No longer creating volumes of air for tolerances
- Changed material of cooling pipes from stainless steel to titanium
Some of these changes go towards ATLSWUPGR-91.
Tagging @lshan and @dshope for info. Will upload some checks here before moving this forward, but feel free to have a look at the changes in the meantime. Update: checks were performed and things look good, details in the comments.
Merge request reports
Activity
Ok, I returned to this to do the checks I wanted to do:
- Make sure the code still runs with both the two- and three-ring layout
- Check that my changes (that for example avoids creating volumes of air for tolerance spacing) doesn't affect any positions
Unfortunately, running a simple sim job:
lxplus700:/afs/cern.ch/work/c/cohm/public/HGTD/21.9_latest > cat sim_s3551_volume_debug_wHGTD.sh Sim_tf.py \ --AMI s3551 \ --maxEvents 3 \ --randomSeed '201015' \ --DataRunNumber '242000' \ --preInclude "all:InDetSLHC_Example/preInclude.NoTRT_NoBCM_NoDBM.py,InDetSLHC_Example/preInclude.SLHC.py,InDetSLHC_Example/preInclude.SLHC_Setup.py,InDetSLHC_Example/preInclude.SLHC_Setup_Strip_GMX.py,SimulationJobOptions/preInclude.CalHits.py,SimulationJobOptions/preInclude.ParticleID.py,preInclude.VolumeDebugger_HGTD.py" \ --preExec 'EVNTtoHITS:from AthenaCommon.DetFlags import DetFlags; DetFlags.HGTD_setOn()' \ --HGTDOn True \ --inputEVNTFile /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/SimCoreTests/ttbar_muplusjets-pythia6-7000.evgen.pool.root \ --outputHITSFile myHITS.pool.root 2>&1 | tee sim_s3551_volume_debug_wHGTD.logfails in the latest nightly:
EVNTtoHITS 07:32:36 GeoModelSvc.SCT... INFO Lorentz angle service retrieved: LorentzAngleSvc = ServiceHandle('SCTLorentzAngleSvc') EVNTtoHITS 07:32:36 GeoModelSvc INFO GeoModelSvc.SCT_GMX_DetectorTool SZ= 158100Kb Time = 4.26S EVNTtoHITS 07:32:36 HGTD_IDDetDescrCnv INFO in createObj: creating a HGTD_ID helper object in the detector store EVNTtoHITS 07:32:36 HGTD_ID INFO Initialize from dictionary EVNTtoHITS 07:32:36 AtlasDetectorID INFO initialize_from_dictionary - OK EVNTtoHITS 07:32:36 HGTD_ID FATAL HGTD_ID::initLevelsFromDict - unable to find hgtd region index: id, reg 2/-1 0 EVNTtoHITS 07:32:36 HGTD_IDDetDescrCnv ERROR Unable to initialize HGTD_ID EVNTtoHITS 07:32:36 DataProxy WARNING accessData: conversion failed for data object 79264207/HGTD_ID EVNTtoHITS 07:32:36 Returning NULL DataObject pointer EVNTtoHITS 07:32:36 GeoModelSvc.HGT... FATAL /build2/atnight/localbuilds/nightlies/Athena/21.9/athena/HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/src/HGTD_DetectorTool.cxx:72 (StatusCode HGTD_DetectorTool::create(StoreGateSvc*)): code 0: detStore->retrieve(idHelper, "HGTD_ID") EVNTtoHITS 07:32:36 StatusCodeSvc FATAL Unchecked StatusCode in HGTD_DetectorTool::create(StoreGateSvc*) from lib /cvmfs/atlas-nightlies.cern.ch/repo/sw/21.9_Athena_x86_64-centos7-gcc62-opt/2021-02-08T2141/Athena/21.9.16/InstallArea/x86_64-centos7-gcc62-opt/lib/libHGTD_GeoModelLib.so EVNTtoHITS 07:32:40 ./runwrapper.EVNTtoHITS.sh: line 2: 31910 Aborted (core dumped) athena.py runargs.EVNTtoHITS.py SimuJobTransforms/skeleton.EVGENtoHIT_ISF.py PyJobTransforms.trfExe.execute 2021-02-09 07:32:40,027 INFO EVNTtoHITS executor returns 134 PyJobTransforms.trfExe.validate 2021-02-09 07:32:41,011 ERROR Validation of return code failed: EVNTtoHITS got a SIGABRT signal (exit code 134) (Error code 65) PyJobTransforms.trfExe.validate 2021-02-09 07:32:41,060 INFO Scanning logfile log.EVNTtoHITS for errors in substep sim PyJobTransforms.transform.execute 2021-02-09 07:32:41,091 CRITICAL Transform executor raised TransformValidationException: EVNTtoHITS got a SIGABRT signal (exit code 134); Logfile error in log.EVNTtoHITS: "HGTD_ID FATAL HGTD_ID::initLevelsFromDict - unable to find hgtd region index: id, reg 2/-1 0" PyJobTransforms.transform.execute 2021-02-09 07:32:44,446 WARNING Transform now exiting early with exit code 65 (EVNTtoHITS got a SIGABRT signal (exit code 134); Logfile error in log.EVNTtoHITS: "HGTD_ID FATAL HGTD_ID::initLevelsFromDict - unable to find hgtd region index: id, reg 2/-1 0")Script and full log here:
/afs/cern.ch/work/c/cohm/public/HGTD/21.9_latest/(succeeding job with this branch on top is here:/afs/cern.ch/work/c/cohm/public/HGTD/21.9/run/).For reasons I haven't been able to work out, the same job runs when compiling this branch against the latest nightly, though I don't think any of the changes should affect the identifiers in any way.
I'm afraid this is all I have time for today (packed meeting schedule most of today and tomorrow..). Any ideas, @dshope?
For info: thanks to @nstyles we also have a new geometry tag which will include the latest ITk changes:
ATLAS-P2-ITK-23-00-03. I think it would be great to also update some of the HGTD content in the db in this tag (importantly cooling pipe material steel --> titanium, and perhaps a few things we've had hardcoded overrides for in the s/w). Ideally this would allow going for the three-ring layout, but that depends on when we can get the identifiers ready for the three-ring layout. Any idea when you think you'd be able to get to it, @dshope? I know you have other things with high priority on your plate, so if you can outline what needs doing maybe someone else can help too!Best, Christian
This merge request affects 1 package:
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel
This merge request affects 3 files:
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/CMakeLists.txt
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/src/HGTD_DetectorFactory.cxx
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/src/HGTD_DetectorTool.cxx
Resolved by Nicholas StylesScript and full log here:
/afs/cern.ch/work/c/cohm/public/HGTD/21.9_latest/(succeeding job with > this branch on top is here:/afs/cern.ch/work/c/cohm/public/HGTD/21.9/run/).For reasons I haven't been able to work out, the same job runs when compiling this branch against > the latest nightly, though I don't think any of the changes should affect the identifiers in any way.
I'm afraid this is all I have time for today (packed meeting schedule most of today and tomorrow..). Any ideas, @dshope?
I think this is a consequence of the move to the static dictionary files. The HGTD parts of the dictionary are not there in the static versions, while I suspect that when compiling locally there is perhaps still an old on-the-fly generated version which is hanging around somewhere?
I think there are two choices:
-
Add the HGTD section in to the existing static files at the end (I guess the 2-ring is a simple subset of the 3-ring from a dictionary perspective(?), so it should be the 3-ring version added if you want both to work still)
-
Get the HGTD dictionary correct in the version for ATLAS-P2-ITK-23-00-03, since I think we anyway want to use a new static file for that, so that we can set the
dictionaryVersiontoITkHGTDand have this properly defined by the geometry tag/dictionary file rather than forcing it in configuration, as we do now.
I think the second option is better, unless there is some reason not to shift HGTD development to be on top of ATLAS-P2-ITK-23-00-03 rather than the older tags?
UPDATE: So, I tried to do the second option since we were anyway making the new geo tag for ITk, and you can see the MR here: !40473 (merged)
The HGTD part was copied from an earlier auto-generated dictionary, but unfortunately it still doesn't seem to work. I think @dshope will have to take a look. I think we can go ahead with the MR anyway, since the ITk part should be fine, and then we can just try and fix the HGTD part in-place.
Edited by Nicholas Styles-
- Last reply by Christian Ohm
-
Athena externals cmake make 
required tests optional tests Full details available on this CI monitor view
Hi all,
Sorry for the slow follow-up on the side of the HGTD readout geometry, I've been otherwise a bit pre-occupied recently with other tasks as alluded to by @cohm above - but I've been following all the recent developments (including the switch to usage of a static identifier dictionary) and should be able to devote some time to help finalise things for the 3-ring layout.
About the error message above - indeed, this is the same printout that we encountered last week when we changed the name of the dictionary version and was fixed by @ncalace in !40230 (merged) where this wasn't properly propagated when initializing the HGTD part of the dictionary. So in general, this error message seems to hint that there's just a problem with reading from any of the HGTD parts of the dictionary (which would also appear if now a static dictionary is being used which doesn't yet have an HGTD part) rather than a more subtle issue with specific identifiers.
Given the fact that we're using the eta and phi indices in the HGTD identifier definitions, I don't think I can say with certainty that the 2-ring layout is just a simple subset of the 3-ring layout from a dictionary perspective. However, I can propose the following steps in order to try and get the identifiers up and running in the 3-ring layout:
This is the relevant information that was encoding the identifiers needed for the 2-ring layout in the auto-generated dictionaries: https://gitlab.cern.ch/atlas/athena/-/blob/21.9/InnerDetector/InDetDetDescr/InDetTrackingGeometryXML/src/HGTD_IdDictSvc.cxx#L95-107
It's basically just hard-coding how many phi indices there are for each eta index (separated for front and back sides of the disks). If I can be provided with how these lists would change for the 3-ring layout (should be doable from looking at how the module volumes are being created in the detector factory), then I can do the additional work of generating a new HGTD section of the static dictionary to test out (already just with the 3-ring layout).
Best, David
-
Thanks for the summary and plan, @dshope - this looks like a good path forward to me! I think @lshan should be best positioned to easily provide the details you need listed above. Note that as the three-ring layout is also very much WIP, the number of modules per row will likely change in the future, so if we can foresee that and reduce the work we need to do to conform to future adjustments, we will thank ourselves.
If this will take time, maybe the hardcoded dict for the two-ring layout could be introduced in the mean time?
Finally, basically nothing in the current db content for HGTD will be different for the two-ring and three-ring layouts - it mainly concerns larger cylindrical main components, and components that build up the individual sensors. So if (and I say if) we really want to keep support for the two-ring layout, we should figure out a way to get the right hardcoded dict through a python switch for a given global tag. If this is too messy, I wonder if we should just drop support for the two-ring layout as I have so far not heard a compelling argument why we need to support it (so if you know of one, please speak up!). @nstyles and @ncalace might have thoughts here!
Best, Christian
Hello,
I would be in favour of supporting what @cohm proposes. As the project moves forward I don't see any reason to continue supporting the two-ring layout. In this way we can directly move to something that will be more stable for future developments as well.
Cheers, Noemi
mentioned in merge request !40520 (merged)
-
So if (and I say if) we really want to keep support for the two-ring layout, we should figure out a way to get the right hardcoded dict through a python switch for a given global tag. If this is too messy, I wonder if we should just drop support for the two-ring layout as I have so far not heard a compelling argument why we need to support it
!40520 (merged) should help with this I hope - the option added there to force a local on-the-fly dictionary instead of the static one, so as long as the on-the-fly generation works for the two-ring (this part is not so clear to me), this would allow it to be run if needed, while the static version can just be the three-ring version.
I tested the above as follows:
Sim_tf.py --AMI s3622 --inputEVNTFile /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/CommonInputs/ttbar_muplusjets-pythia6-7000.evgen.pool.root --outputHITSFile myHITS.pool.root --maxEvents 3 --HGTDOn True --geometryVersion ATLAS-P2-ITK-23-00-02_VALIDATION --preExec 'from InDetSLHC_Example.SLHC_JobProperties import SLHC_Flags;SLHC_Flags.ForceDictionaryGeneration = True'and it seemed to work nicely. Also, the local file generated was identical to the one here, so as soon as we switch to
ATLAS-P2-ITK-23-00-03which uses that version, hopefully this should work. I'm not sure why my previous tests (as mentioned above) on that file failed, but hopefully it can be confirmed soon (once the new geometry tag is in the DDDB) that all is fine with this. I guess this is still the two-ring that is encoded in the file though? We can anyway update that part later "in place" for the three-ring.And for completeness (@lshan this is what I was mentioning in the meeting) this command should work in
21.9,latestnow that the new geometry tag is available:Sim_tf.py --AMI s3622 --inputEVNTFile /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/CommonInputs/ttbar_muplusjets-pythia6-7000.evgen.pool.root --outputHITSFile myHITS.pool.root --maxEvents 3 --HGTDOn True --geometryVersion ATLAS-P2-ITK-23-00-03_VALIDATIONwhen I run with this I see in the output:
================================================================================ Mem Size Disk Size Size/Evt MissZip/Mem items (X) Container Name (X=Tree|Branch) ================================================================================ 2.340 kb 0.746 kb 0.249 kb 0.000 3 (T) DataHeader -------------------------------------------------------------------------------- [...] 108.353 kb 73.092 kb 24.364 kb 0.000 3 (B) SiHitCollection_p4_HGTD_Hitswhich looks encouraging.
Edited by Nicholas Styles added 339 commits
-
192795cb...41635c5d - 338 commits from branch
atlas:21.9 - 8090bbf7 - Merge branch '21.9' of https://gitlab.cern.ch:8443/atlas/athena into 21.9-HGTD-geo-improvements
-
192795cb...41635c5d - 338 commits from branch
-
Hi all,
Ok, it took me a while to get back to this, but I think I managed to do the checks I wanted to thanks to the changes and instructions mentioned above.
- I used the same sim job I've been using before (so s3551 and old ITk tag), but added the preExec to generate the dict on the fly.
- I printed out the z positions of the main components to ensure I didn't shift anything around when introducing spaces instead of air-filled volumes for the tolerances.
- Things look just fine, so I think this can move forward!
More details below, perhaps mostly for my own notes.
Best, Christian
lxplus7112:/afs/cern.ch/work/c/cohm/public/HGTD > grep -B3 "local z =" 21.9_latest/run/sim_s3551_volume_debug_wHGTD.log | cut -d "O" -f 2 > ComponentPositionssWithoutChanges.log lxplus7112:/afs/cern.ch/work/c/cohm/public/HGTD > grep -B3 "local z =" 21.9/run/sim_s3551_volume_debug_wHGTD.log | cut -d "O" -f 2 > ComponentPositionssWithChanges.log lxplus7112:/afs/cern.ch/work/c/cohm/public/HGTD > cat ComponentPositionssWithChanges.log R = 860 mm R = 890 mm R = 920 mm HGTD::Moderator HGTD::BackCover ( sct::CFiberSupport ), local z = 38.5 mm, Rmin = 120 mm, Rmax = 1100 mm, DZ = 4 mm HGTD::ModeratorIn ( LAr::BoratedPolyethelyne ), local z = 17.5 mm, Rmin = 120 mm, Rmax = 900 mm, DZ = 15 mm HGTD::ModuleLayer3 ( std::Air ), local z = -1.25 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::SupportPlate ( std::Aluminium ), local z = -5.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::CoolingPlate ( sct::CFiberSupport ), local z = -9 mm, Rmin = 120 mm, Rmax = 920 mm, DZ = 3 mm HGTD::SupportPlate ( std::Aluminium ), local z = -12.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::ModuleLayer2 ( std::Air ), local z = -16.75 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::ModuleLayer1 ( std::Air ), local z = -26.25 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::SupportPlate ( std::Aluminium ), local z = -30.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::CoolingPlate ( sct::CFiberSupport ), local z = -34 mm, Rmin = 120 mm, Rmax = 920 mm, DZ = 3 mm HGTD::SupportPlate ( std::Aluminium ), local z = -37.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::ModuleLayer0 ( std::Air ), local z = -41.75 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::FrontCover ( sct::CFiberSupport ), local z = -55 mm, Rmin = 120 mm, Rmax = 1000 mm, DZ = 7.5 mm HGTD::InnerRCover ( sct::CFiberSupport ), local z = -10 mm, Rmin = 115 mm, Rmax = 120 mm, DZ = 52.5 mm HGTD:: -- R = 860 mm R = 890 mm R = 920 mm HGTD::Moderator HGTD::BackCover ( sct::CFiberSupport ), local z = 38.5 mm, Rmin = 120 mm, Rmax = 1100 mm, DZ = 4 mm HGTD::ModeratorIn ( LAr::BoratedPolyethelyne ), local z = 17.5 mm, Rmin = 120 mm, Rmax = 900 mm, DZ = 15 mm HGTD::ModuleLayer3 ( std::Air ), local z = -1.25 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::SupportPlate ( std::Aluminium ), local z = -5.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::CoolingPlate ( sct::CFiberSupport ), local z = -9 mm, Rmin = 120 mm, Rmax = 920 mm, DZ = 3 mm HGTD::SupportPlate ( std::Aluminium ), local z = -12.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::ModuleLayer2 ( std::Air ), local z = -16.75 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::ModuleLayer1 ( std::Air ), local z = -26.25 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::SupportPlate ( std::Aluminium ), local z = -30.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::CoolingPlate ( sct::CFiberSupport ), local z = -34 mm, Rmin = 120 mm, Rmax = 920 mm, DZ = 3 mm HGTD::SupportPlate ( std::Aluminium ), local z = -37.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 0.5 mm HGTD::ModuleLayer0 ( std::Air ), local z = -41.75 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 3.75 mm HGTD::FrontCover ( sct::CFiberSupport ), local z = -55 mm, Rmin = 120 mm, Rmax = 1000 mm, DZ = 7.5 mm HGTD::InnerRCover ( sct::CFiberSupport ), local z = -10 mm, Rmin = 115 mm, Rmax = 120 mm, DZ = 52.5 mm HGTD:: lxplus7112:/afs/cern.ch/work/c/cohm/public/HGTD > diff ComponentPositionssWithoutChanges.log ComponentPositionssWithChanges.log 6d5 < HGTD::ToleranceBack ( std::Air ), local z = 33.5 mm, Rmin = 120 mm, Rmax = 900 mm, DZ = 1 mm 13d11 < HGTD::ToleranceMid ( std::Air ), local z = -21.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 1 mm 19d16 < HGTD::ToleranceFront ( std::Air ), local z = -46.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 1 mm 29d25 < HGTD::ToleranceBack ( std::Air ), local z = 33.5 mm, Rmin = 120 mm, Rmax = 900 mm, DZ = 1 mm 36d31 < HGTD::ToleranceMid ( std::Air ), local z = -21.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 1 mm 42d36 < HGTD::ToleranceFront ( std::Air ), local z = -46.5 mm, Rmin = 120 mm, Rmax = 660 mm, DZ = 1 mmThe commands I ran (after changing the print-outs of cyldindrical volumes being created to INFO level (for some reason couldn't get debug printouts):
lxplus7112:/afs/cern.ch/work/c/cohm/public/HGTD > cat 21.9_latest/run/sim_s3551_volume_debug_wHGTD.sh Sim_tf.py \ --AMI s3551 \ --maxEvents 3 \ --randomSeed '201015' \ --DataRunNumber '242000' \ --preInclude "all:InDetSLHC_Example/preInclude.NoTRT_NoBCM_NoDBM.py,InDetSLHC_Example/preInclude.SLHC.py,InDetSLHC_Example/preInclude.SLHC_Setup.py,InDetSLHC_Example/preInclude.SLHC_Setup_Strip_GMX.py,SimulationJobOptions/preInclude.CalHits.py,SimulationJobOptions/preInclude.ParticleID.py,preInclude.VolumeDebugger_HGTD.py" \ --preExec 'EVNTtoHITS:from AthenaCommon.DetFlags import DetFlags; DetFlags.HGTD_setOn();from InDetSLHC_Example.SLHC_JobProperties import SLHC_Flags;SLHC_Flags.ForceDictionaryGeneration = True;from AthenaCommon.AppMgr import ServiceMgr as svcMgr' 'svcMgr.MessageSvc.defaultLimit = 0' \ --HGTDOn True \ --inputEVNTFile /cvmfs/atlas-nightlies.cern.ch/repo/data/data-art/SimCoreTests/ttbar_muplusjets-pythia6-7000.evgen.pool.root \ --outputHITSFile myHITS.pool.root 2>&1 | tee sim_s3551_volume_debug_wHGTD.log -
This merge request affects 1 package:
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel
This merge request affects 3 files:
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/CMakeLists.txt
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/src/HGTD_DetectorFactory.cxx
- HighGranularityTimingDetector/HGTD_DetDescr/HGTD_GeoModel/src/HGTD_DetectorTool.cxx
added review-pending-level-1 label
-
Athena externals cmake make required tests optional tests Full details available on this CI monitor view