Conversation
github-actions
bot
added
topic: infrastructure
|
@wdconinc Probably should finish this one. Maybe, you can do the review this and I can implement remaining changes? |
wdconinc
left a comment
wdconinc
left a comment
There was a problem hiding this comment.
I think this looks good enough. We may want to work on a CI job or benchmark to see how quickly we break this functionality.
| /> | ||
| <position x="50*cm" y="50*cm" z="LFHCAL_length - LFHCALElectronicsThickness" /> | ||
| </eightmodule> | ||
| <fourmodule name="4MModule" vis="InvisibleWithDaughters" repeat="2"> |
There was a problem hiding this comment.
We can probably just as well strip the fourmodule definition.
There was a problem hiding this comment.
We probably could indeed. However for the different test beams they might still end up being ever so slightly different due to different versions of the geometry , in particular already the 8M geom (i.e for the 2023 test beam we only had 14 layers, while for the 2024 we had 64 and for 2025 we'll have 60).
There was a problem hiding this comment.
sorry missed the context yes for the 8M module only we can definitely strip the 4M definitions. ... Still slightly asleep
| <readouts> | ||
| <readout name="LFHCALHits"> | ||
| <segmentation type="NoSegmentation"/> | ||
| <id>system:8,moduleIDx:6,moduleIDy:6,moduletype:1,passive:1,towerx:2,towery:1,rlayerz:4,layerz:4</id> |
There was a problem hiding this comment.
Wondering if we need to pull the segmentation out of this and stick it in a shared file. The goal of the TB is to be able to use the same reconstruction for TB as for all of ePIC. That means strong coupling between the segmentations used in both.
There was a problem hiding this comment.
the problem with that might be that at least for the current TB we have a higher z-segmentation, so I am not 100% sure we can use exactly the same bit distribution (I have to try it out).
| // 8M module specific loading | ||
| xml_comp_t eightM_xml = detElem.child(_Unicode(eightmodule)); | ||
| xml_dim_t eightMmod_dim = eightM_xml.dimensions(); | ||
| moduleParamsStrct eightM_params( |
There was a problem hiding this comment.
Filling struct from xml_comp_t would be a useful helper function to reduce code duplication.
There was a problem hiding this comment.
Do you have an example where this is done?
There was a problem hiding this comment.
src/DD4hepDetectorHelper.h is probably the best example within our code base.
| <define> | ||
| <include ref="lfhcal/module_definitions.xml"/> | ||
| <constant name="Mod_MountingPlateThickness" value="0* mm"/> | ||
| <constant name="Mod_PCBLength" value="135* cm"/> |
There was a problem hiding this comment.
This produces a new warning:
WARN +++ Object 'Mod_PCBLength' is already defined. New value will be ignored
WARN +++ Object 'Mod_MountingPlateThickness' is already defined. New value will be ignored
(happens to be what confuses the convert-to-step on CI)
We need to move those two out from module_definitions.xml to lfhcal.xml (and keep in lfhcal_2024_TB and lfhcal_2025_TB, of course).
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
FriederikeBock
force-pushed
the
pr/lfhcal_8m_module_only
branch
from
70634da to
c908b5d
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
3 participants
name: Pull request
about: Create a pull request to merge bug fixes or new features
title: ''
labels: ''
assignees: ''
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
Does this PR change default behavior?