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

Removal of Phase 2 Tracker geometries: T6, T14, T16, T17, T19, T20 #30512

Closed
tsusa opened this issue Jul 2, 2020 · 30 comments
Closed

Removal of Phase 2 Tracker geometries: T6, T14, T16, T17, T19, T20 #30512

tsusa opened this issue Jul 2, 2020 · 30 comments

Comments

@tsusa
Copy link
Contributor

tsusa commented Jul 2, 2020

At the moment, there are eight different Phase 2 Tracker geometries in the CMSSW (https://github.com/cms-sw/cmssw/blob/master/Configuration/Geometry/README.md).

Several geometries are kept for the reproducibility of previous results presented in different TDRs. Due to the overhead in maintaining the conditions for different layouts, we would like to remove several Tracker geometries.

After discussing the proposal in the last Tracker phase2 simulation meeting
[https://indico.cern.ch/event/879046/contributions/3702950/attachments/2068831/3472638/news20200703.pdf], the updated proposal (target CMSSW 11.2.X) is:

  • first round of clean-up

    • remove T6 (MTD TDR), T14 (L1 TDR)
  • second round of clean-up

    • remove T16, T17, T19, T20

T15 (HLT TDR) geometry is not affected by the clean-up. T19 and T20, which are development geometries used internally by the Tracker phase2 WG, will be eventually superseded by versions with the latest mechanical drawings, still in the 11.2.X cycle.

Could you please let us if any of the geometries to be remove is still needed and why. Please also note that Tracker layouts are parts of different D geometries which should be migrated in case they are still needed.

@mmusich @emiglior @skinnari @ghugo83

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 2, 2020

A new Issue was created by @tsusa Tatjana Susa.

@Dr15Jones, @silviodonato, @dpiparo, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Jul 3, 2020

assign geometry, upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2020

New categories assigned: geometry,upgrade

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@kpedro88,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 3, 2020

@tsusa I applaud the initiative in cleaning up geometries. However, at the moment, this has to be approached with some care.

In particular, we cannot remove T15 any time soon. It is used for the HLT TDR, for which production is still ongoing. Though the new production is not planning to use 11_2_X (current development cycle), relevant PRs are still being backported to 11_1_X. Those PRs need to be tested in the master branch with the desired production geometry, so it needs to be retained. Further, developers and analyzers typically want to compare new results to the latest TDR geometry for quite a while after the official TDR production.

Personally, I would be happy to remove the others. However, we should also consider which geometries that use those tracker versions can be removed entirely, rather than migrating. This will require some coordination with the HGCal, MTD, and Muon DPGs. I will launch those discussions ASAP in order to converge on a cleanup plan.

@rekovic should also comment if there's a concrete reason to keep the L1T TDR geometry in 11_2_X.

@tsusa
Copy link
Contributor Author

tsusa commented Jul 3, 2020

@kpedro88 I agree that it needs to be done carefully; we open the issue for that reason. We would like to remove as many layouts as possible, however the most critical is T6, as it causes issue in the L1 tracking integration.

@tsusa tsusa changed the title Removal of Phase 2 Tracker geometries: T6, T14, T15, T16 Removal of Phase 2 Tracker geometries: T6, T14, T16, T17, T19, T20 Jul 8, 2020
@kpedro88
Copy link
Contributor

kpedro88 commented Jul 8, 2020

@tsusa I see you've edited the proposal. Questions/comments:

  • What's the reason to keep T16 in the first round of cleanup?
  • Can we finally get rid of T5? This was kept because of some kind of post-TDR-review paranoia, but isn't even in any active detector geometries.
  • There are a lot of development geometries for other subdetectors built on top of T17/T19. It introduces a lot of churn to have to update these multiple times as new tracker geometries come out. The typical procedure is to vary one subdetector at a time while doing development. (In other words, I would propose keeping T17/T19 in 11_2_X, rather than introduce another ~5 geometry variations and possibly cause unnecessary discrepancies in ongoing studies.)
    • We could consider getting rid of T19 and migrating the D55 geometry that tests M5 to use T17. Running this by the Muon DPG now.

Here is the summary of feedback from the other DPGs:

  • HGCal: remove C4, C6, C8
    • replace D47 w/ equivalent using M4, O4
  • Muon: remove M2, M3
  • MTD: remove I5, I7, I9
  • common: remove O2, O3

Once the tracker list is finalized, I'll prepare a PR.

@mmusich
Copy link
Contributor

mmusich commented Jul 9, 2020

@kpedro88

What's the reason to keep T16 in the first round of cleanup?

we think it is safe to remove T16 in the 1st round.

Can we finally get rid of T5? This was kept because of some kind of post-TDR-review paranoia, but isn't even in any active detector geometries.

Indeed we can drop T5, especially if re-activating T5 in a recent CMSSW release implies all the issues recently faced when introducing L1 Tracking with T6

There are a lot of development geometries for other subdetectors built on top of T17/T19. It introduces a lot of churn to have to update these multiple times as new tracker geometries come out. The typical procedure is to vary one subdetector at a time while doing development. (In other words, I would propose keeping T17/T19 in 11_2_X, rather than introduce another ~5 geometry variations and possibly cause unnecessary discrepancies in ongoing studies.)

The main reason for superseding T17 and T19 is that these layouts have still thin OT sensors while the decision of having thick OT sensors was endorsed in Fall 2019 (and thick sensors are the only option from sensors vendors...)

@ghugo83
Copy link
Contributor

ghugo83 commented Jul 9, 2020

Hi,

Presently, from Phase 2 Tracker point of view, all latest designs/features are fully contained in T19 and T20.
If it is finally allowed to remove the others, that's great :)
What we propose is to remove as many geometries as possible which are now unused + also remove T19 and T20 + add 3 new geometries which will contain all the latest designs/features (and new ones ;p): T21, T22 and T23.

As discussed with @emiglior and @skinnari, I will first do a PR to:

  • Remove T6, T14, T16, T17 and superseed them by T20.
  • Add T21, T22, T23 [1].

Then, another PR which will (once T21 and T23 are validated):

  • Remove T19 and superseed it by T23 (3D).
  • Optional: Remove T20 and superseed it by T21 (baseline).

Please let me know if there is any geometry planned to be removed which should actually not be / or any diff to to this plan ;p. Great if a PR can be done first to remove potential dependencies with other subdetectors.

[1] What I will put in T21, T22 and T23:

  • T21 will be the new Phase 2 Tracker baseline.
    It will include OT with thick sensors + latest updates in TBPS and TEDD (from mechanical design). Its IT will have 25x100 sensors.
  • T22 will be based on T21, but with 50x50 sensors in IT.
  • T23 will be based on T21, but with 3D sensors in IT.
    This allows to introduce a new Phase 2 Tracker baseline (T21) + be able to compare the different (major) options for the IT.

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 9, 2020

@ghugo83 please do not make a PR yet, as there is not agreement for this plan. I would much rather do the (initial) removal myself, as I am the one coordinating with all of the DPGs. Geometry removals do not just affect tracker.

Tracker is welcome to add more versions in a separate PR, but removing versions that are part of geometries actively in use by other DPGs is not recommended. This can cause a lot of unnecessary churn and disruption: physics results would change for reasons unrelated to the other detector versions actually being studied.

Here is an updated plan (first step) taking into account the needs of all DPGs, not just tracker:

  • Tracker: remove T5, T6, T14, T16, T19
  • HGCal: remove C4, C6, C8
    • replace D47 w/ equivalent using M4, O4
  • Muon: remove M2, M3
    • replace D55 w/ equivalent using T17
  • MTD: remove I5, I7, I9
  • common: remove O2, O3

If it's preferred to keep T19 in the first step, I would still make the D55 replacement so it's easier to remove T19 later.

Swapping T17 to T20 to T2X just interferes with HGCal development for no reason. The tracker DPG will have to live with keeping T17 for a while. It's great for the tracker to update their baseline geometry to be more realistic, but other DPGs have to continue their own studies, as well.

@ghugo83
Copy link
Contributor

ghugo83 commented Jul 9, 2020

@kpedro88 This is what I understood of the plan. Also, there has been the recent need to add an extra geometry (T21 instead of T20), which added to the mess. As mentioned, you are more than welcome to deal with the dependencies of the other subdetectors first :)
If you remove T19, what are you going to superseed it with? Can't superseed a Tracker with 3D sensors with one with planar ;)

@emiglior
Copy link
Contributor

emiglior commented Jul 9, 2020

@kpedro88

If it's preferred to keep T19 in the first step, I would still make the D55 replacement so it's easier to remove T19 later.

I would ask to keep T19 in the first step as currently it is the only geometry available for the 3D sensors developments

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 9, 2020

@ghugo83 right now only two geometries use T19: the one for testing T19, and one for testing M5. I've proposed to the muon DPG that we replace T19 with T17 in the M5 geometry, to reduce dependencies on T19. T17 is already used in many geometries, so needs to be kept for a while.

To be completely clear, in my first step PR, I plan to remove T5, T6, T14, T16, as well as the other subdetector geometries. T19 will be kept as just requested.

@ghugo83
Copy link
Contributor

ghugo83 commented Jul 9, 2020

@kpedro88 Great, yes things make sense to me now. Please go ahead and i ll add T21, T22 and T23 later :)

@kpedro88
Copy link
Contributor

see #30645 for the first step with removals

@kpedro88
Copy link
Contributor

@mmusich @emiglior to continue the discussion of decommissioning T17:

There are currently 6 detector configurations in that use T17 (Configuration/Geometry).

Two of these (D61 and D62) have already been moved (in the last cleanup) from T19 to T17 to make it easier to deprecate T19 (though this hasn't happened yet).

D61 (evolution of D55) is supposed to be compared to D51 with fewer muon overlaps (M5 vs M4). But the muon geometry has continued to evolve since then, so maybe this is no longer useful. @bsunanda what do you think?

D62 and three others (D57, D58, D59) are all configurations for different HGCal development geometries. For these, it's useful to avoid any non-HGCal-related differences. While they could in principle all be migrated to T21, it's possible this would disrupt ongoing studies (@bsunanda @cseez @rovere). In addition, repeated migrations introduce a lot of seemingly unnecessary churn.

D51 is just D49+T17, so can be dropped.

What is the exact motivation for decommissioning T17? Does its continued existence create a particular burden or block a specific development?

@emiglior
Copy link
Contributor

Adding @skinnari
We want to remove T17, T19 and T20 as they correspond to a layout of the Outer Tracker (featuring thin sensors) which in practice will never be built.
Also we foresee short/medium term developments in the description of the OT (namely, the description of the signal shape and of inefficiencies in the PS-p) which will be made/tuned specifically for the thick sensors option.

@kpedro88
Copy link
Contributor

@bsunanda @slomeo please comment ASAP on whether M5 and D61 are still needed

@bsunanda @cseez @rovere please comment ASAP on whether it's acceptable to HGCal for development workflows D57, D58, D59, D62 to be migrated from T17 to T21
(as a side note, this could make D63 redundant, as it has T21+C11+M4, rather than T17/T21+C11+M6)

@bsunanda
Copy link
Contributor

@kpedro88 @slomeo M5 is M4 but without overlap. I believe you have to retain M4 in view of D49. If you retain D63/D57 or D64 please use M6 rather than M4 there.

@bsunanda
Copy link
Contributor

@kpedro88 @cseez @rovere C11, C13 and C14 geometries are for V12, V13 and V14. I think V13, V14 are experimental at this moment. So changing tracker geometry to T21 will be OK for D57, D59 and D62

@bsunanda
Copy link
Contributor

@mariadalfonso @kpedro88 C10 and C12 are 2 versions of HFNose with slightly modified support structure. They are in D60 and D58 respectively. I think both can make use of T21. Maria can comment if one of them can be avoided.

@slomeo
Copy link
Contributor

slomeo commented Sep 28, 2020

@kpedro88 : I got in touch with Muon DPGO and I sent you the emails. I think that you can proceed.

@mariadalfonso
Copy link
Contributor

@mariadalfonso @kpedro88 C10 and C12 are 2 versions of HFNose with slightly modified support structure. They are in D60 and D58 respectively. I think both can make use of T21. Maria can comment if one of them can be avoided.

yes, we can move to the T21 in both D60 and D58 (HFnose scenarios)

@kpedro88
Copy link
Contributor

@emiglior I'll implement the migrations on top of your branch, once #31541 is merged.

@emiglior
Copy link
Contributor

@kpedro88 OK. Just to be sure, I will not rebase myself the branch in my PR #31572

@kpedro88
Copy link
Contributor

kpedro88 commented Oct 5, 2020

@kpedro88
Copy link
Contributor

kpedro88 commented Oct 7, 2020

+upgrade

@srimanob
Copy link
Contributor

srimanob commented May 4, 2021

I believe this can be closed, @qliphy @silviodonato

@mmusich
Copy link
Contributor

mmusich commented May 4, 2021

@cms-sw/geometry-l2 please sign.

@cvuosalo
Copy link
Contributor

cvuosalo commented May 4, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2021

This issue is fully signed and ready to be closed.

@qliphy qliphy closed this as completed May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests