-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
experimental ILD models for FCCee #390
Conversation
@Victor-Schwan was heavily involved |
You probably want to pick up also this LumiCal change: #391 Have you considered putting the ILD FCCee model into the FCCee directory? This might make its existence a bit more obvious :) |
@Zehvogel I had the same thought, we'll discuss it in ILD; maybe at least link it to the FCC directory. |
Looks like merging #388 introduced a conflict here (as far as I can see, it should be straight forward to fix by dropping the first commit). Did you also start to check whether this can be put into the |
68c6eb6
to
0bfafa7
Compare
|
Yeah, this is what I was expecting. I think symlinking the |
still to do: fix materials definitions in which give rise to warnings:
have contacted expert (Gerald Grenier) |
Since nothing in this PR changes the materials definitions, those are pre-existing, right? In that case, I would prefer to fix that in a separate PR. |
OK, seems a good plan |
942086c
to
6fe7825
Compare
Minor request from my side: Could the contents of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I fully understand why the vis
attributes need to be renamed, resp. where the collision is coming from. Shouldn't that just be an issue in case a vis
property that is used by any detector is not defined? What am I missing here?
We linked subdetectors from CLD. In the XML file of the CLD Inner Tracker |
I see, that makes sense. Thanks. I suppose there is no easy way of leaving the other models untouched, e.g. by copying the |
There is certainly scope for sharing colour definitions among different k4geo models, but it would take some effort... |
What is still missing to lift this out of the WIP status that it's currently in? |
#381 introduces changes to the MDI e.g. to the beampipe in the common_MDI v00. We rebased on top of that PR and linked several files there. Hence, we thought to merge the other PR first. I am not sure whether the changes are needed though |
@Victor-Schwan are you sure we are based on that PR? I think it works fine on the master branch (eg the lumical is taken from the CLD directory). If that MDI PR goes through, we would update to profit from improvements/changes. |
I checked again and initially, I rebased another branch with |
I think it's stated quite obviously in the provided README and to really make sure we could also put it into the release notes once more. For me this seems to be good as it is, also because there will likely be some iteration still once we have reconstruction available. So, as soon as you remove the WIP I would be fine to have a last look and merge. |
WIP removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to merge from my side. Paging @BrieucF and @andresailer to make sure that they are also fine with putting this under the FCC umbrella.
This is fine by me! |
…ng around of defs)
…nking to xml descriptions from other detectors (eg RedVis -> ILD_RedVis)
…ions between v01, v02 models.
This reverts commit 4653909. (adjustment of CellID encoding)
Co-authored-by: Thomas Madlener <[email protected]>
12d1336
to
ce8d676
Compare
BEGINRELEASENOTES
-- MDI from FCCee common MDI area
-- vertex & lumi cal identical to CLD_o2_v07
-- for ILD_FCCee_v01: inner tracker now based on TrackerBarrel_o1_v06 (adapted from CLD_o2_v07), TPC inner radius slightly increased 329 -> 365 mm to avoid MDI envelope
-- for ILD_FCCee_v02: inner tracker identical to CLD_o2_v07, TPC inner radius significantly increased 329 -> 701 mm to accommodate larger inner tracker
-- no ECAL ring, LHCAL, beamcal
-- other subdetectors same as ILD_l5_v02
-- some moving/renaming of definitions in ILD_common_v02 was necessary
ENDRELEASENOTES