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

Drop of D49-D85 Phase-2 geometry #38594

Closed
srimanob opened this issue Jul 4, 2022 · 36 comments
Closed

Drop of D49-D85 Phase-2 geometry #38594

srimanob opened this issue Jul 4, 2022 · 36 comments

Comments

@srimanob
Copy link
Contributor

srimanob commented Jul 4, 2022

Following the HGCAL proposal in
https://docs.google.com/document/d/1fCIpJJQLw0vGCKyR9LRmFmWVcAQZOlHk5jK-ESQLJnQ/edit?usp=sharing,
they propose to preserve only V16 and V17 of HGCAL (== C17-C19).

So the affected geometries include

  • D49 = T15+C9+M4+I10+O4+F2 (HLT TDR baseline)
  • D60 = T15+C10+M4+I10+O4+F3 (With HFNose)
  • D68 = T21+C11+M6+I11+O5+F4 (For HGCAL study on evolution of detector)
  • D70 = T21+C13+M7+I11+O6+F6 (For HGCAL study on evolution of detector)
  • D76 = T21+C14+M9+I13+O7+F6
  • D77 = T24+C14+M9+I13+O7+F6 (Current default scenario)
  • D80 = T25+C14+M9+I13+O7+F6
  • D81 = T26+C14+M9+I13+O7+F6
  • D82 = T21+C15+M9+I13+O7+F7
  • D83 = T24+C16+M9+I13+O7+F6
  • D84 = T24+C13+M7+I11+O6+F6 (For HGCAL study on evolution of HGCal replacing D70)
  • D85 = T24+C14+M9+I14+O7+F6

This issue is to collect feedback, or need if other version of geometry should be preserved.

The plan is to implement this proposal when the next release series start, i.e. 12_6_X.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2022

A new Issue was created by @srimanob Phat Srimanobhas.

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

cms-bot commands are listed here

@srimanob
Copy link
Contributor Author

srimanob commented Jul 4, 2022

FYI @kpedro88 @bsunanda @AdrianoDee

@trtomei @fwyzard
Do you still need to keep D49 for HLT?

@srimanob
Copy link
Contributor Author

srimanob commented Jul 4, 2022

assign upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2022

New categories assigned: upgrade

@AdrianoDee,@srimanob you have been requested to review this Pull request/Issue and eventually sign? Thanks

@srimanob
Copy link
Contributor Author

srimanob commented Jul 4, 2022

FYI @cms-sw/simulation-l2 @cms-sw/hgcal-dpg-l2

@kpedro88
Copy link
Contributor

kpedro88 commented Jul 4, 2022

  • will need to add a new scenario with HF nose (compatible with one of the latest HGCal geometries)
  • @srimanob @AdrianoDee we may want to take this opportunity to reset the workflow numbers... it's possibly been three years since we last did this in Clean up Phase2 workflows #27449

@fabiocos
Copy link
Contributor

fabiocos commented Jul 5, 2022

from the MTD point of view keeping just scenario I15 is ok, this will allow us to drop support in the code for obsolete scenarios (like single disc ring-shaped ETL in D49), simplifying the structure. Some initial cleaning is already proposed for BTL in #38589 (not needing this proposal), in preparation of a new BTL geometry (and MTD scenario) to be added in the coming weeks/months.

@srimanob
Copy link
Contributor Author

srimanob commented Jul 6, 2022

  • will need to add a new scenario with HF nose (compatible with one of the latest HGCal geometries)

This is handled in #38596 (Thanks @bsunanda)

@srimanob srimanob changed the title Drop of C9-C16 Phase-2 geometry Drop of D49-D85 Phase-2 geometry Jul 11, 2022
@srimanob
Copy link
Contributor Author

I've made a draft PR, so ppl can check:
#38702

@kpedro88
Copy link
Contributor

@srimanob what do you think about resetting the workflow numbers?

@srimanob
Copy link
Contributor Author

Hi @kpedro88
Yeah, that is what I would like to ask you. If D86 will be the first workflow, should it just be 20000.0 after resetting?

Currently, the first upgrade workflow after cleaning will be
38600.0 2026D86+FourMuPt_1_200_pythia8_GenSimHLBeamSpot+DigiTrigger+RecoGlobal+HARVESTGlobal

@kpedro88
Copy link
Contributor

Yes; in particular, you should change this line as follows: numWFConflict = [[25000,26000],[50000,51000]]

@srimanob
Copy link
Contributor Author

OK. That line I updated already in the PR, but just to reflect the drop of workflows. I will update again.

@cbotta
Copy link
Contributor

cbotta commented Jul 13, 2022

Hello. The L1T team would like to ask if the D49 could be kept. It can be useful for validation studies on the HLT-TDR samples (that we are using for L1T developments since a year). And I think HLT Upgrade would echo this.

@srimanob
Copy link
Contributor Author

@cbotta Do you have an idea on how long will you need it? Could you please provide more information on the schedule of development?

The current plan is to drop by the next release series, i.e. 12_6 at the end of August.

@cbotta
Copy link
Contributor

cbotta commented Jul 14, 2022

@srimanob After discussing with the team: we will very likely have our new MC production in 12_5. We think it is useful to keep the D49 up to this release, so that we have one release that can run on both MC productions. But after that we will switch to the new samples, and so it is fine to remove it from 12_6. Thanks!

@srimanob
Copy link
Contributor Author

@cbotta
This is great, thanks. I will add this bullet to tomorrow SIM meeting.

@srimanob
Copy link
Contributor Author

Hi @cbotta @cms-sw/l1-l2
May I come back to the question on dropping geometries. It is unclear to me as we are not done yet the L1T dev, could you please comment.

FYI @bsunanda @cms-sw/simulation-l2 @cms-sw/hcal-dpg-l2

@cbotta
Copy link
Contributor

cbotta commented Sep 19, 2022

Hi. My understanding is that L1T is still targeting 12_5_0 for the MC production, therefore my previous comment still holds. @cecilecaillol

@srimanob
Copy link
Contributor Author

Hi @cbotta
Thanks. The question is if we can handle the cleaning in master (12_6) now, or we should wait a bit until all L1T devs converge.

@cbotta
Copy link
Contributor

cbotta commented Sep 19, 2022

If possible I would wait for @cecilecaillol to confirm that all what we need can be ported to 12_5, and we can start the production there. Should be happening soon

@srimanob
Copy link
Contributor Author

@cbotta @cecilecaillol @cms-sw/l1-l2
After the simulation meeting today, there are requests to merge the clean up PR #38702 asap. D49 will not be supported anymore in 12_6.

From your plan, it should be OK as all L1T code will be backported to 12_5, so 12_5 will be the release which support both D49 and D88. If there is an objection, please raise it here before the next release meeting (27 Sep). Thanks for understanding.

FYI @bsunanda @fabiocos

@cecilecaillol
Copy link
Contributor

We should have a 12_5_1 release for our MC production so this is fine from our side

@srimanob
Copy link
Contributor Author

srimanob commented Sep 29, 2022

Unit test failures after merging cleaning PR:

  • Validation/Geometry/test/runP_HGCal_cfg.py
  • Geometry/TrackerGeometryBuilder/test/python/testPixelTopologyMapTest_cfg.py
  • SimTracker/TrackerMaterialAnalysis/test/trackingMaterialProducer10GeVNeutrino_ForPhaseII.py
  • SLHCUpgradeSimulations/Geometry/test/phase2_digi_reco_pixelntuple_cfg.py
  • SLHCUpgradeSimulations/Geometry/test/writeFile_phase2_cfg.py
  • CondTools/SiPixel/test/SiPixelTemplateDBObjectUploader_Phase2_cfg.py

They call D49 or D76. This should be updated to D88 or later.

FYI @cms-sw/geometry-l2 @cms-sw/trk-dpg-l2
Should I update all to D88? If OK, I can provide the PR. Thanks.

@bsunanda
Copy link
Contributor

bsunanda commented Sep 29, 2022 via email

@mmusich
Copy link
Contributor

mmusich commented Sep 29, 2022

@srimanob I suppose you can also clean up the T* versioned autoCond Global Tags for the Tracker geometries that have been removed?

activeDets = ["T15","T21","T25","T26","T30"]
phase2GTs = {}
for det in activeDets:
appendedTags = ()
for key in activeKeys:
if (det in allTags[key]):
appendedTags += allTags[key][det]
else :
pass
phase2GTs["phase2_realistic_"+det] = ('phase2_realistic', appendedTags)

@cms-sw/alca-l2

@srimanob
Copy link
Contributor Author

@srimanob I suppose you can also clean up the T* versioned autoCond Global Tags for the Tracker geometries that have been removed?

activeDets = ["T15","T21","T25","T26","T30"]
phase2GTs = {}
for det in activeDets:
appendedTags = ()
for key in activeKeys:
if (det in allTags[key]):
appendedTags += allTags[key][det]
else :
pass
phase2GTs["phase2_realistic_"+det] = ('phase2_realistic', appendedTags)

@cms-sw/alca-l2

Hi @mmusich @cms-sw/alca-l2
Sure, I can include in my PR. Thanks for information.

@mmusich
Copy link
Contributor

mmusich commented Sep 29, 2022

Please use D92 (or the later version with new Tracker and MTD) and not D88.

I see a couple of problems:

Footnotes

  1. from DAS $ dasgoclient -query='dataset dataset=/RelValSingleMuPt10/*2026D92*/GEN-SIM' | wc -l is 0

@srimanob
Copy link
Contributor Author

srimanob commented Sep 29, 2022

Let's go for D88 in unit tests until we validate D92 and move it to baseline, and have everything ready.

@mmusich
Copy link
Contributor

mmusich commented Sep 29, 2022

@srimanob

PR #39535 should take care of these:

Geometry/TrackerGeometryBuilder/test/python/testPixelTopologyMapTest_cfg.py
SimTracker/TrackerMaterialAnalysis/test/trackingMaterialProducer10GeVNeutrino_ForPhaseII.py
SLHCUpgradeSimulations/Geometry/test/phase2_digi_reco_pixelntuple_cfg.py
SLHCUpgradeSimulations/Geometry/test/writeFile_phase2_cfg.py

as for the others:

Validation/Geometry/test/runP_HGCal_cfg.py

I would let it to HGCAL experts (FYI @cms-sw/hgcal-dpg-l2 )

while

CondTools/SiPixel/test/SiPixelTemplateDBObjectUploader_Phase2_cfg.py

requires more involved changes, including an update of the cms-data externals, that I would leave to the Pixel CPE experts (@tvami @SanjanaSekhar)

@tvami
Copy link
Contributor

tvami commented Sep 29, 2022

CondTools/SiPixel/test/SiPixelTemplateDBObjectUploader_Phase2_cfg.py
requires more involved changes, including an update of the cms-data externals, that I would leave to the Pixel CPE experts (@tvami @SanjanaSekhar)

Sure I'll have a look

@mmusich
Copy link
Contributor

mmusich commented Sep 29, 2022

changing topic, shouldn't be https://github.com/cms-sw/cmssw/blob/master/Configuration/Geometry/README.md also updated to remove the unsupported geometries?

@srimanob
Copy link
Contributor Author

changing topic, shouldn't be https://github.com/cms-sw/cmssw/blob/master/Configuration/Geometry/README.md also updated to remove the unsupported geometries?

Thx. I think i forgot it whilt waiting the PR to converge. I will update it together with workflow id resetting.

@srimanob
Copy link
Contributor Author

srimanob commented Oct 4, 2022

@srimanob
Copy link
Contributor Author

srimanob commented Oct 6, 2022

Staring from CMSSW_12_6_X_2022-10-05-1100, unit tests seem to be OK now.

@srimanob
Copy link
Contributor Author

srimanob commented Nov 3, 2022

Done.

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

9 participants