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
Deprecating Phase2 Tracker T3 and corresp. scenarios + Adding D18 (= D8 but with the non deprec. Tracker) #18852
Conversation
A new Pull Request was created by @boudoul (boudoul) for master. It involves the following packages: Configuration/Geometry @civanch, @Dr15Jones, @kkousour, @ianna, @mdhildreth, @fabozzi, @cmsbuild, @franzoni, @kpedro88, @GurpreetSinghChahal, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
numWFIB.extend([20434.0,20434.2]) #2023D10 | ||
numWFIB.extend([21234.0,21234.2]) #2023D4 | ||
numWFIB.extend([21234.1]) #2023D4 special tracking workflow | ||
numWFIB.extend([23234.0]) #2023D8 - already has timing |
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.
Need to add D18 workflows here
@@ -45,12 +33,12 @@ | |||
# pre-generation of WF numbers | |||
numWFStart={ | |||
2017: 10000, | |||
2023: 20000, | |||
2023: 20400,#GB: since the WF numbers are appended from this and I want to skip this one I increased this number to the new actual one |
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 would prefer to keep this as the nominal 20000 and just use the numWFConflict
to skip it
upgradeProperties[2023]['2023D16PU'] = deepcopy(upgradeProperties[2023]['2023D16']) | ||
upgradeProperties[2023]['2023D16PU']['ScenToRun'] = ['GenSimHLBeamSpotFull','DigiFullPU','RecoFullGlobalPU', 'HARVESTFullGlobalPU'] | ||
upgradeProperties[2023]['2023D17PU'] = deepcopy(upgradeProperties[2023]['2023D17']) | ||
upgradeProperties[2023]['2023D17PU']['ScenToRun'] = ['GenSimHLBeamSpotFull','DigiFullPU','RecoFullGlobalPU', 'HARVESTFullGlobalPU'] | ||
upgradeProperties[2023]['2023D18PU'] = deepcopy(upgradeProperties[2023]['2023D18']) |
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.
you don't need to have D18 twice in this list
@boudoul sent a few comments for you it's fine with me to reuse D8 workflow numbers for D18, we did the same thing during a previous round of deprecation. |
thanks for the super prompt feedback, I will adress your comments asap |
Pull request #18852 was updated. @civanch, @Dr15Jones, @kkousour, @ianna, @mdhildreth, @fabozzi, @cmsbuild, @franzoni, @kpedro88, @GurpreetSinghChahal, @davidlange6 can you please check and sign again. |
for year in upgradeKeys: | ||
for i in range(1,len(upgradeKeys[year])): |
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 think this could be fixed a bit more simply/efficiently:
for i in range(0,len(upgradeKeys[year])):
numWFtmp = numWFStart[year] if i==0 else (numWFAll[year][i-1] + numWFSkip)
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.
@boudoul can you implement this change?
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.
hi @kpedro88 , my last commit implemented the changes you proposed
a case not protected against here is that of two excluded regions that are neighboring. other than that, indeed, they should be the same
… On May 19, 2017, at 6:54 PM, Kevin Pedro ***@***.***> wrote:
@kpedro88 commented on this pull request.
In Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py:
> for year in upgradeKeys:
- for i in range(1,len(upgradeKeys[year])):
I think this could be fixed a bit more simply/efficiently:
for i in range(0,len(upgradeKeys[year])):
numWFtmp = numWFStart[year] if i==0 else (numWFAll[year][i-1] + numWFSkip)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@davidlange6 the (somewhat fragile) assumption is that people will combine neighboring excluded regions in the |
("O1","T5","C3","M2","F1","I1") : "D16", | ||
("O2","T5","C3","M2","F2","I1") : "D17", | ||
("O1","T5","C2","M1","F1","I2") : "D18", |
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.
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.
D18 is just intended to be D8 with T5 (not used for TDR). Other changes/updates can happen at a later time.
Pull request #18852 was updated. @civanch, @Dr15Jones, @kkousour, @ianna, @mdhildreth, @fabozzi, @cmsbuild, @franzoni, @kpedro88, @GurpreetSinghChahal, @davidlange6 can you please check and sign again. |
Pull request #18852 was updated. @civanch, @Dr15Jones, @kkousour, @ianna, @mdhildreth, @fabozzi, @cmsbuild, @franzoni, @kpedro88, @GurpreetSinghChahal, @davidlange6 can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
+1 |
@ianna , @boudoul , could this PR be the cause of failing unittest in 92X IBs https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_9_2_X_2017-05-29-2300/unitTestLogs/DetectorDescription/RegressionTest
can you please check and provide a fix? |
hi @smuzaffar , thanks for reporting- I just made a PR #19006 which should cure this, I let @ianna comments though . Thanks |
I notice that workflow numbers are not in order of D* anymore: D18 comes before D17. Was there a good reason for that? Also, the flat tracker was never in the short matrix tests. why is it added now? |
@slava77 they weren't in order before, either. We replaced D8 with D18, as we had done in the past for some other workflows when deprecating T1. The track trigger worked with T3, but doesn't yet work with T5. In order to make sure that section of the code doesn't break, I decided to add D10 (w/ T4) to the matrix tests. |
right, and now that this PR is merged, I will proceed with the track trigger with T5 |
Greetings
This PR aims to deprecate all scenarios which contain the tracker T3 (I also removed all corresponding T3 xmls) , keeping of course all the ones with the T5 Tracker .
Basically it implies to remove the following scenario :
D7,D4,D8,D9, D12, D13, D15
and therefore keep :
D10, D11, D14,D16,D17
Note that for any remaining scenario the numbering in runthematrix has been preserved (hopefully, it looks ok in my tests...)
@kpedro88 kindly made me noticed that the timing layer was only in D8 (D8 is removed in this PR ) so I followed his suggestion and I have implemented a new scenario D18 which is equivalent to D8 but with the new tracker so that this software will still be there (but with the right tracker) .
For D18 , I choosed to re-use the same WF numbers as D8 , hope this is fine.
Comments and suggestions about this (pretty huge) cleaning are of course welcome , thanks.