81X Track Trigger primitives update #15075

sviret · 2016-06-30T07:52:25Z

This PR contains bugfixes for the TTClusters and TTStubs production in CMSSW 81X. It also contains the corresponding TT Associator classes and recipes/necessary scripts to run this production within a Tracker-Only geometry

cmsbuild · 2016-06-30T07:52:54Z

A new Pull Request was created by @sviret (Seb Viret) for CMSSW_8_1_X.

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
L1Trigger/TrackTrigger
SLHCUpgradeSimulations/Configuration
SimTracker/TrackTriggerAssociation

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @sdevissc, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @threus, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

civanch · 2016-06-30T16:16:32Z

@sviret , in TTStubAlgorithm-official.h, TTStubBuilder.h, TTClusterAssociationMap.h, TTClusterAssociator.h, TTStubAssociator.h substantial pieces of code are inlined, they should be moved to corresponding source files.

In TTClusterAssociator.h atuo_ptr is used - it is unsafe and should be replaced by std::unique_ptr or std::shared_ptr.

In TTClusterAssociationMap and in TTStubAssociator.h cout and cerr are used, they should be replaced by edm::logInfo, edm::logWarning, or edm::logError. There is "exit(0)" which should be removed.

ebrondol · 2016-06-30T20:43:41Z

Hi @sviret, I am a bit worried about the copy-paste cfg files that you have done in this pr (phase2TKOnlyTracker, etc..) . @boudoul did you have already approve it?
I am working in this moment to the tracking phase2 transition to Era. Not sure that the situation that we are converging is the best..
@delaere @kpedro88

kpedro88 · 2016-06-30T20:53:56Z

@sviret As @ebrondol suggests, I am a priori opposed to further mass cloning of files. Please justify the need for tkOnly geometries and customization functions (the latter of which are now discouraged in general and we should try to avoid introducing more of them except on a very temporary basis).

sviret · 2016-06-30T22:40:32Z

Tracker only geometry is useful for track trigger community, in order to quickly produce heavy pu samples with detailed persistency.
Now, I can understand the need to avoid code duplication. In order to address that I can move the customization stuff for tk only in existing customize scripts (flat and tilted). This will be transparent for the rest of the custom function.

For the new geometry files, I'm not sure i can use the classic xml files (we are loading SD info already at this level), but I can give it a try.

ianna · 2016-07-04T08:45:22Z

Configuration/Geometry/python/GeometryExtended2023TkOnlysimReco_cff.py

+import FWCore.ParameterSet.Config as cms
+
+# Ideal geometry, needed for transient ECAL alignement
+from Configuration.Geometry.GeometryExtended2023TkOnlysim_cff import *


@sviret - IMHO, it would be nice to have consistent capitalization in new file names (e.g. geometry scenarios): TkOnlySim and TkOnlyTilted.
BTW, Shouldn't TkOnlySim be TkOnlyFlat (as contrary to Tilted, rather then contrary to Reco)?

cmsbuild · 2016-07-04T12:36:38Z

Pull request #15075 was updated. @civanch, @mdhildreth, @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please check and sign again.

sviret · 2016-07-04T12:43:22Z

@civanch, I tried to implement most of the comments. Didn't managed to changed the std::auto_ptr by std::unique or std::shared (got some error message at the compilation).

@ebrondol, @kpedro88, @boudoul, @delaere, I tried to remove most of the addons related to the Tk-Only geometry. I just added some dedicated customization functions in the existing Phase2TkXXX.py customizations scripts (transparent for normal running), and the geometry stuff is fully contained into one single py script now contained in the TrackTrigger/python directory (one script per geometry flavour). So, to summarize, the only packages affected by this PR are now L1Trigger/TrackTrigger and SimTracker/TrackTriggerAssociation.

sviret · 2016-07-04T12:44:26Z

To be complete, SLHCUpgradesimulation/Configuration is also affected

ebrondol · 2016-07-04T12:49:45Z

@sviret thanks a lot for the update.
@kpedro88 have you any idea on how to deal with these changes when we move to era? If so, there is something that can be already done in this PR?

cmsbuild · 2016-07-04T20:28:06Z

Pull request #15075 was updated. @civanch, @mdhildreth, @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please check and sign again.

cmsbuild · 2016-07-04T21:06:16Z

Pull request #15075 was updated. @civanch, @mdhildreth, @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please check and sign again.

cmsbuild · 2016-07-26T18:39:36Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15075/14237/summary.html

civanch · 2016-07-28T10:03:17Z

+1

kpedro88 · 2016-07-29T14:22:10Z

@rekovic, @mulhearn - please review & sign when you can

kpedro88 · 2016-08-01T15:08:06Z

@rekovic, @mulhearn ping

kpedro88 · 2016-08-03T16:06:34Z

@davidlange6 - reminder to comment and/or merge...

davidlange6 · 2016-08-03T17:21:37Z

I didn’t forget

On Aug 3, 2016, at 7:00 PM, Kevin Pedro <[email protected]mailto:[email protected]> wrote:

@davidlange6 https://github.com/davidlange6 - reminder to comment and/or merge...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/15075#issuecomment-237283638, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw7H075E5ze6mljgQhuA86BARcNWsks5qcMlCgaJpZM4JB5H5.

davidlange6 · 2016-08-05T14:07:53Z

L1Trigger/TrackTrigger/plugins/TTStubBuilder.cc

+    const PixelTopology* top0 = dynamic_cast< const PixelTopology* >( &(pix0->specificTopology()) );
+    const int chipSize = 2 * top0->rowsperroc(); /// Need to find ASIC size in half-strip units
+
+    std::map< int, std::vector< TTStub< Ref_Phase2TrackerDigi_ > > > moduleStubs; /// Temporary storage for stubs before max check


@sviret @kpedro88 - stepping in for the L1 guys after some delay - not sure I get throw all of the comments now, but will start

Here - since you are searching this map every stub - pls use an unordered_map (though eventually I guess other solutions may need to be considered)

kpedro88 · 2016-08-08T21:30:34Z

@sviret ETA on answering review comments?

sviret · 2016-08-30T07:43:59Z

@kpedro88 just back to work this week, I will address review comments this week.

sviret · 2016-08-30T12:09:55Z

Seems that changes have been made in L1Trigger/L1TrackTrigger package in august (#15404). This is creating some conflicts with this PR. Should I close this PR and start a new one from scratch?

kpedro88 · 2016-08-30T14:25:00Z

@sviret please try to rebase this PR before starting a closing/reopening/merging cycle. Some basic instructions are here: https://twiki.cern.ch/twiki/bin/view/CMS/SWGuideHATS2016Git#CMSSW_rebasing_recipe

sviret · 2016-08-30T15:14:44Z

@kpedro88, I'm totally ignorant in git, so might have done something wrong. I based myself on an official release for the PR (810_pre7), so I naively did:

git rebase --onto CMSSW_8_1_X_2016-08-30-1100 CMSSW_8_1_0_pre7 81X_TTPrimitives

which failed miserably, as I was expecting. The next command of the tuto, with start and end, is all greek to me, so I didn't even tried it.

I won't have lot of time to spent on that (I was actually not even supposed to do this work...), so even if I understand that it's not very satisfying starting a new PR is much simpler for me, unless there is some magic command to rebase things nicely, which I doubt is existing as one of the main the package involved in this PR has been modified in the meantime (without informing the people working on it, BTW)...

kpedro88 · 2016-08-30T15:27:03Z

@sviret you will have to elaborate on "failed miserably". I just tried this procedure (merged your branch into pre7 and then ran the exact command you listed there), and it appears to proceed fine. There are conflicts that have to be solved manually before the rebase continues, but that is part of the process.

If you just want to redo everything from scratch, fine, but it is a waste of the power and flexibility of Git.

Some more info on resolving conflicts: when you get a message like this

CONFLICT (content): Merge conflict in SLHCUpgradeSimulations/Configuration/python/combinedCustoms.py
Failed to merge in the changes.
Patch failed at 0003 Adding recipes for tracker-only stub production
The copy of the patch that failed is found in:
   /uscms_data/d3/pedrok/phase2/CMSSW_8_1_0_pre7/src/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

You can do git status -s and see which file has a conflict (the one marked UU):

...
A  L1Trigger/TrackTrigger/test/SLHC_PGUN_TkOnly_TILTED_81X.py
UU SLHCUpgradeSimulations/Configuration/python/combinedCustoms.py
A  SLHCUpgradeSimulations/Configuration/python/phase2TkOnlyFlat.py
...

Git automatically inserts merge conflict markers (example) to denote the conflicting changes. You should edit the file to resolve the conflict as needed. Then:

git add SLHCUpgradeSimulations/Configuration/python/combinedCustoms.py
git rebase --continue

By following this procedure, Git does most of the work for you.

sviret · 2016-08-30T15:35:07Z

@kpedro88. By failing miserably I mean that it's not doing what I want in 5min... In particular, the problem is that apparently the values I entered in the command are wrong, because for example phase2TkOnlyFlat.py has been removed in the last commits of the PR, so I don't know why it's appearing here. It looks has if it's loading an older version of the branch. There is certainly a good reason for git to do that, but I'm missing it, of the command I entered is simply wrong

kpedro88 · 2016-08-30T15:48:05Z

@sviret a rebase applies each commit you made in this branch, in order, on top of the specified IB tag. So if a later commit removes that file, it will still be removed by that later commit once the rebase gets to that point.

If you think the commit history is too messy to be worth doing all of that, you can squash the commits first:

git reset --soft HEAD~13
git commit -m "add track trigger stuff"

Then use the rebase command and it should only identify conflicts in the latest version of your changes. (The number 13 is there because you have 13 commits. git reset --soft undoes the commit(s), but keeps all the changes.)

sviret · 2016-09-01T08:31:26Z

@kpedro88, tried again today, with your instructions, but this time I see new files appearing with conflicts which have nothing to do with the PR.
As I said already, I don't have time to spent on that, I will close the PR and make a new one from the latest IB, much simpler for me.

Seb added 6 commits June 24, 2016 14:16

Adding validate TTPrimitives software in CMSSW 81X

4f1758d

Adapt the TTStub builder to the tilted geometry

9db03d9

Adding recipes for tracker-only stub production

0af5b9f

Adding PU recipes

b6b6440

Correct root dict file for the associator

4f90422

Bug fix in the PU recipe

488f490

cmsbuild added this to the Next CMSSW_8_1_X milestone Jun 30, 2016

cmsbuild added simulation-pending l1-pending geometry-pending pending-signatures tests-pending orp-pending comparison-pending labels Jun 30, 2016

ianna reviewed Jul 4, 2016
View reviewed changes

Impplementation of comments, removal of TkOnly external dependancies

600c17c

cmsbuild removed the geometry-pending label Jul 4, 2016

Bug fix in TTStubAssociationMap

7122ff7

Update TTStubAssociationMap.h

5b83d0e

cmsbuild added comparison-available and removed comparison-pending labels Jul 26, 2016

cmsbuild added simulation-approved and removed simulation-pending labels Jul 28, 2016

davidlange6 reviewed Aug 5, 2016
View reviewed changes

ebrondol mentioned this pull request Aug 10, 2016

Second part of the migration to era for phase2 tracking #15417

Merged

sviret closed this Sep 1, 2016

sviret mentioned this pull request Sep 1, 2016

Track trigger primitives in 81X #15691

Closed

sviret deleted the 81X_TTPrimitives branch September 26, 2016 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

81X Track Trigger primitives update #15075

81X Track Trigger primitives update #15075

sviret commented Jun 30, 2016

cmsbuild commented Jun 30, 2016

civanch commented Jun 30, 2016 •

edited

Loading

ebrondol commented Jun 30, 2016

kpedro88 commented Jun 30, 2016

sviret commented Jun 30, 2016

ianna Jul 4, 2016

cmsbuild commented Jul 4, 2016

sviret commented Jul 4, 2016

sviret commented Jul 4, 2016

ebrondol commented Jul 4, 2016

cmsbuild commented Jul 4, 2016

cmsbuild commented Jul 4, 2016

cmsbuild commented Jul 26, 2016

civanch commented Jul 28, 2016

kpedro88 commented Jul 29, 2016

kpedro88 commented Aug 1, 2016

kpedro88 commented Aug 3, 2016

davidlange6 commented Aug 3, 2016

davidlange6 Aug 5, 2016

kpedro88 commented Aug 8, 2016

sviret commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Sep 1, 2016

81X Track Trigger primitives update #15075

81X Track Trigger primitives update #15075

Conversation

sviret commented Jun 30, 2016

cmsbuild commented Jun 30, 2016

civanch commented Jun 30, 2016 • edited Loading

ebrondol commented Jun 30, 2016

kpedro88 commented Jun 30, 2016

sviret commented Jun 30, 2016

ianna Jul 4, 2016

Choose a reason for hiding this comment

cmsbuild commented Jul 4, 2016

sviret commented Jul 4, 2016

sviret commented Jul 4, 2016

ebrondol commented Jul 4, 2016

cmsbuild commented Jul 4, 2016

cmsbuild commented Jul 4, 2016

cmsbuild commented Jul 26, 2016

civanch commented Jul 28, 2016

kpedro88 commented Jul 29, 2016

kpedro88 commented Aug 1, 2016

kpedro88 commented Aug 3, 2016

davidlange6 commented Aug 3, 2016

davidlange6 Aug 5, 2016

Choose a reason for hiding this comment

kpedro88 commented Aug 8, 2016

sviret commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Aug 30, 2016

kpedro88 commented Aug 30, 2016

sviret commented Sep 1, 2016

civanch commented Jun 30, 2016 •

edited

Loading