Skip to content
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

Merged
merged 30 commits into from
Oct 17, 2024
Merged

Conversation

danieljeans
Copy link
Contributor

@danieljeans danieljeans commented Sep 12, 2024

BEGINRELEASENOTES

  • Fix severe overlaps in ILD_l5_v11 inner tracker (outer barrel layer)
  • Introduce new experimental models ILD_FCCee_v01 and ILD_FCCee_v02 under FCCee/ILD_FCCee directory
    -- 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

@danieljeans
Copy link
Contributor Author

@Victor-Schwan was heavily involved

@Zehvogel
Copy link
Contributor

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 :)

@danieljeans
Copy link
Contributor Author

@Zehvogel I had the same thought, we'll discuss it in ILD; maybe at least link it to the FCC directory.

@tmadlener
Copy link
Contributor

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 FCCee/ILD_FCCee (or similar) either fully living there or via some clever symlinking?

@danieljeans
Copy link
Contributor Author

  • fixed the conflicts (some missing definition...)
  • moved the ILD_FCCee_v01 model to the FCCee directory. Sym-linking is non-trivial because main description files live at different depths under FCCee/ and ILD/ , which seems to create problems with relative paths...

@tmadlener
Copy link
Contributor

Sym-linking is non-trivial because main description files live at different depths under FCCee/ and ILD/ , which seems to create problems with relative paths...

Yeah, this is what I was expecting. I think symlinking the ILD_common_v02 to FCCee/ILD_FCCee is probably the best we can do, unless we want to explicitly copy the contents over.

@danieljeans
Copy link
Contributor Author

still to do: fix materials definitions in
ILD/compact/ILD_common_v02/materials.xml

which give rise to warnings:

Warning in <TGeoMixture::ComputeDerivedQuantities>: Mixture RPCGAS2: sum of weights is: 0.99249
Warning in <TGeoMixture::ComputeDerivedQuantities>: Mixture PEEK-GF30: sum of weights is: 1.03333
Warning in <TGeoMixture::ComputeDerivedQuantities>: Mixture g10-RPC: sum of weights is: 1.10025

have contacted expert (Gerald Grenier)

@tmadlener
Copy link
Contributor

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.

@danieljeans
Copy link
Contributor Author

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

@tmadlener
Copy link
Contributor

Minor request from my side: Could the contents of the README.mds that are currently in the v01 and v02 folders be put into one top level README.md in the compact top level folder? I think that would make it easier to see what the different models represent on a quick glance.

Copy link
Contributor

@tmadlener tmadlener left a 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?

ILD/compact/ILD_common_v02/display.xml Show resolved Hide resolved
ILD/compact/ILD_common_v02/display.xml Show resolved Hide resolved
ILD/compact/README.md Outdated Show resolved Hide resolved
ILD/compact/ILD_common_v02/materials.xml Outdated Show resolved Hide resolved
FCCee/ILD_FCCee/compact/ILD_FCCee_v01/FCCdefs_common.xml Outdated Show resolved Hide resolved
@Victor-Schwan
Copy link
Contributor

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 RedVis is defined (see https://github.com/key4hep/k4geo/blob/main/FCCee%2FCLD%2Fcompact%2FCLD_o2_v07%2FInnerTracker_o2_v08.xml#L70) which causes a conflict with the definition in ILD_common_v02. Hence, we decided to rename ILDs vis attributes.

@tmadlener
Copy link
Contributor

We linked subdetectors from CLD. In the XML file of the CLD Inner Tracker RedVis is defined (see https://github.com/key4hep/k4geo/blob/main/FCCee%2FCLD%2Fcompact%2FCLD_o2_v07%2FInnerTracker_o2_v08.xml#L70) which causes a conflict with the definition in ILD_common_v02. Hence, we decided to rename ILDs vis attributes.

I see, that makes sense. Thanks. I suppose there is no easy way of leaving the other models untouched, e.g. by copying the display.xml to an ILD_FCCee_common folder, and removing the clashes? Would that not work because then the ILD parts would be missing some of the vis attibutes?

@danieljeans
Copy link
Contributor Author

I see, that makes sense. Thanks. I suppose there is no easy way of leaving the other models untouched, e.g. by copying the display.xml to an ILD_FCCee_common folder, and removing the clashes? Would that not work because then the ILD parts would be missing some of the vis attibutes?

There is certainly scope for sharing colour definitions among different k4geo models, but it would take some effort...

ILD/compact/README.md Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

What is still missing to lift this out of the WIP status that it's currently in?

@Victor-Schwan
Copy link
Contributor

Victor-Schwan commented Oct 9, 2024

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

@danieljeans
Copy link
Contributor Author

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.
In my understanding, we're ready to remove WIP here (on the understanding that they are experimental models and not an "officially blessed" version of ILD).

@Victor-Schwan
Copy link
Contributor

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. In my understanding, we're ready to remove WIP here (on the understanding that they are experimental models and not an "officially blessed" version of ILD).

I checked again and initially, I rebased another branch with v11 on the common mdi PR. I did not see any problem with this PR. Sorry for the confusion

@tmadlener
Copy link
Contributor

ready to remove WIP here (on the understanding that they are experimental models and not an "officially blessed" version of ILD)

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.

@danieljeans danieljeans changed the title WIP: ILD models for FCCee experimental ILD models for FCCee Oct 16, 2024
@danieljeans
Copy link
Contributor Author

WIP removed

Copy link
Contributor

@tmadlener tmadlener left a 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.

@BrieucF
Copy link
Contributor

BrieucF commented Oct 16, 2024

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!

danieljeans and others added 27 commits October 17, 2024 11:02
…nking to xml descriptions from other detectors (eg RedVis -> ILD_RedVis)
This reverts commit 4653909.
(adjustment of CellID encoding)
Co-authored-by: Thomas Madlener <[email protected]>
@tmadlener tmadlener enabled auto-merge (squash) October 17, 2024 09:03
@tmadlener tmadlener merged commit ac3dca9 into key4hep:main Oct 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants