81X Track Trigger primitives update

[sviret]

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]

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]

@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]

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]

@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]

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]

@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]

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

[sviret]

@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]

To be complete, SLHCUpgradesimulation/Configuration is also affected

[ebrondol]

@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]

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

[cmsbuild]

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

[cmsbuild]

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

[civanch]

+1

[kpedro88]

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

[kpedro88]

@rekovic, @mulhearn ping

[kpedro88]

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

[davidlange6]

I didn’t forget

On Aug 3, 2016, at 7:00 PM, Kevin Pedro <notifications@github.commailto:notifications@github.com> wrote:

@davidlange6https://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]

@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]

@sviret ETA on answering review comments?

[sviret]

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

[sviret]

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]

@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]

@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]

@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]

@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]

@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]

@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.