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

L2muon changes: L2MuonSeedGenerator, L2MuonProducer #1864

Merged
merged 5 commits into from Jan 15, 2014

Conversation

trocino
Copy link
Contributor

@trocino trocino commented Dec 18, 2013

The new code in packages L2MuonSeedGenerator, L2MuonProducer introduces the following options in L2 muon reconstruction for muon triggers:

  • possibility to request a mandatory matching between the L1 muon candidate and a trajectory seed (in L2MuonSeedGenerator);
  • possibility to configure the trajectory builder used in L2 track reconstruction (in L2MuonProducer).

By default, the new code won't affect muon triggers: the new options can only be enabled with a proper configuration in ConfDB, if requested by the TSG.

The standard tests (runTheMatrix) were run successfully in CMSSW_7_0_X_2013-12-16-1400 and CMSSW_7_0_0_pre10. The report can be found here:

http://trocino.web.cern.ch/trocino/Temp/2013-12-17_runTheMatrix/runall-report-step123-.log

In order to run the tests, the new parameters must be added to the following HLT configurations
HLTrigger/Configuration/python/HLT_8E33v2_cff.py
HLTrigger/Configuration/python/HLT_8E33v2_Famos_cff.py
as shown here:
http://trocino.web.cern.ch/trocino/Temp/2013-12-17_runTheMatrix/HLT_8E33v2_cff.py
http://trocino.web.cern.ch/trocino/Temp/2013-12-17_runTheMatrix/HLT_8E33v2_Famos_cff.py

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @trocino (Daniele Trocino) for CMSSW_7_0_X.

L2muon changes: L2MuonSeedGenerator, L2MuonProducer

It involves the following packages:

RecoMuon/L2MuonProducer
RecoMuon/L2MuonSeedGenerator

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77 can you please review it and eventually sign? Thanks.
@bachtis this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

@slava77
Copy link
Contributor

slava77 commented Dec 18, 2013

@perrotta @Martin-Grunewald
Hi Daniele, please contact Martin and Andrea with details of the changes to be made.
(in the past it was the most effective to list the modules with needed param changes/additions)

Also, it's my understanding so far that there should be a set of parameters that keeps the performance roughly unchanged. This set will be used in the frozen menus.
Martin or Andrea can comment better.

Slava

@perrotta
Copy link
Contributor

Hi Slava, Daniele.
As far as I can see, the new code just needs to be parsed in ConfDB, assuming that the default new parameters are the ones that do not change the old behaviour: is it correct Daniele?
If so, while it is nowadays probably a bit too late for pre11, we'll try to produce the modified HLT configuration and have it integrated in the first possible occasion.
Andrea

@trocino
Copy link
Contributor Author

trocino commented Dec 18, 2013

Hi all,
(sorry for the slow answer, it was 3:30 am here when I managed to make the request at last...)
There are two additional parameters to be added w.r.t. the current configuration:

In hltL2MuonSeeds/L2MuonSeedGenerator:
hltL2MuonSeeds.UseUnassociatedL1 = cms.untracked.bool( True )

In hltL2Muons/L2MuonProducer:
hltL2Muons.MuonTrajectoryBuilder = cms.string("StandAloneMuonTrajectoryBuilder")

With these values (which are not default, they need be set this way), the performance is totally unaffected, the triggers will behave exactly as before. It is useful mostly to have the new parameters in release and in the HLT configuration, so that they can be tested by all the PAGs with different values. So probably it's not extremely important if it doesn't make it to pre11.

Let me know if you need some more information.
Thanks! Daniele

@Martin-Grunewald
Copy link
Contributor

Hi,

I see two new parameters, one is untracked with a default of "TRUE" - does that reproduce the old behaviour?
For the second parameter, please use existAs to check for it's presence. If it is not present OR
its value is "", simply do the old behavour (without the LogWarning message).
This way the frozen menus still run, we have the possibility to run the old behaviour with the
new code, and we can eventually change the HLT config to exploit the new features once
this is parsed into ConfDb (but the code with these modifs can already go in),

Best regards

Martin

@Martin-Grunewald
Copy link
Contributor

One other thing: it looks like the value of the new untracked boolean affects the physics outcome, hence it should NOT be untracked but tracked. And then please use the same strategy as for the second, tracked parameter described above.

@cmsbuild
Copy link
Contributor

Pull request #1864 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77 can you please check and sign again.

@trocino
Copy link
Contributor Author

trocino commented Dec 20, 2013

Hi Martin,
done as you suggested.
Daniele

@Martin-Grunewald
Copy link
Contributor

Great, thanks a lot!

@ktf
Copy link
Contributor

ktf commented Dec 20, 2013

Does not merge. Moving to pre12 / 71X.

@slava77
Copy link
Contributor

slava77 commented Jan 13, 2014

@trocino
Daniele, as mentioned by Giulio, this PR will need to be updated so that it merges cleanly into 70X.
Please start with a recent integration build.

Nominally, this PR will go to 71X

@trocino
Copy link
Contributor Author

trocino commented Jan 13, 2014

@perrotta @Martin-Grunewald
Hi Slava, Giulio,
the conflicts were fixed and the request updated.
Daniele

@cmsbuild
Copy link
Contributor

Pull request #1864 was updated. @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

+1

tested c6605eb CMSSW_7_0_X_2014-01-14-0200 (local test area sign295)

Jenkins is still in "Comparison with the baseline Still running..."

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes (tests are also fine). @ktf can you please take care of it?

@nclopezo
Copy link
Contributor

Hi Slava,

I had changed something in the jenkins job and it was not triggering the comparison. Thanks for making me notice. I started it again by hand and you can now see the comparison.

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

Hi David.

Thank you.

We should get back to include the reco fwlite script in the tests run by jenkins.

validateJR.sh relDirNew relDirRef newVSref matrix_70X.txt
#newVSref specifies the name added to the output to mark where it's from
# matrix_70X.txt is manually made map file between short wflow names, input files and process names

the script and map file are in https://github.com/slava77/cms-reco-tools
Output png files are made only if there's a difference
You could check it in your setup and see what would work better with jenkins
... I have your last iteration somewhere, but I'm not sure now if that's worth going back to.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes (tests are also fine). @ktf can you please take care of it?

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2014

@ktf
Giulio, just to make it clear: this is for 70X.
There's nothing affecting RECO here and HLT asked to get these in 70X (the defaults for the new code keep the old behavior).
So, HLT has the final word on need in 70X here.

@Martin-Grunewald
Copy link
Contributor

Hi,

For sure we want it in 70X.

The list of PRs is added to yesterday's wiki:

https://twiki.cern.ch/twiki/bin/view/CMS/OfflineComputingPlanningMeeting20140114

Best regards

Martin

On Wed, 15 Jan 2014, slava77 wrote:

@ktf
Giulio, just to make it clear: this is for 70X.
There's nothing affecting RECO here and HLT asked to get these in 70X (the defaults for the new code keep the old behavior).
So, HLT has the final word on need in 70X here.


Reply to this email directly or view it on GitHub:
#1864 (comment)

Martin

Martin W. Gruenewald e-mail: Martin.Grunewald@cern.ch
http://cern.ch/Martin.Grunewald

ktf added a commit that referenced this pull request Jan 15, 2014
HLT updates -- L2muon changes: L2MuonSeedGenerator, L2MuonProducer
@ktf ktf merged commit f0d971f into cms-sw:CMSSW_7_0_X Jan 15, 2014
@davidlange6
Copy link
Contributor

Any chance that this PR is affecting relvals?

https://hypernews.cern.ch/HyperNews/CMS/get/relval/2833/30.html

@perrotta
Copy link
Contributor

perrotta commented Feb 3, 2014

Hi David.
The issue you are referencing is known to be due to the L1 stuffs in the DT, it has nothing to do with the L2/L3 HLT muons:
https://hypernews.cern.ch/HyperNews/CMS/get/muon-object-validation/499/2.html

@davidlange6
Copy link
Contributor

is there a thread you know of confirming that? This one just says it could be a problem….

On Feb 3, 2014, at 2:15 PM, perrotta notifications@github.com
wrote:

Hi David.
The issue you are referencing is known to be due to the L1 stuffs in the DT, it has nothing to do with the L2/L3 HLT muons:
https://hypernews.cern.ch/HyperNews/CMS/get/muon-object-validation/499/2.html


Reply to this email directly or view it on GitHub.

@perrotta
Copy link
Contributor

perrotta commented Feb 3, 2014

I agree it is an issue and it has to be followed up.

What I said is simply that it starts at L1 see also

https://hypernews.cern.ch/HyperNews/CMS/get/trigger-performance/705/6.html

and the plots linked therein, for instance

https://cmsweb.cern.ch/dqm/relval/start?runnr=1;dataset=/RelValZMM_13/CMSSW_7_0_0_pre12-POSTLS170_V1-v1/DQM;sampletype=offline_data;filter=all;referencepos=overlay;referenceshow=all;referenceobj1=other%3A%3A/RelValZMM_13/CMSSW_7_0_0_pre11-POSTLS162_V4-v1/DQM%3A;referenceobj2=none;referenceobj3=none;referenceobj4=none;search=;striptype=object;stripruns=;stripaxis=run;stripomit=none;workspace=Everything;size=M;root=HLT/Muon/Efficiency_Layouts/HLT_Mu5;focus=;zoom=no;

Thus the culprit cannot be L2 muon code, and in particular cannot be
an effect of this PR.

davidlange6 notifications@github.com ha scritto:

is there a thread you know of confirming that? This one just says it
could be a problem….

On Feb 3, 2014, at 2:15 PM, perrotta notifications@github.com
wrote:

Hi David.
The issue you are referencing is known to be due to the L1 stuffs
in the DT, it has nothing to do with the L2/L3 HLT muons:
https://hypernews.cern.ch/HyperNews/CMS/get/muon-object-validation/499/2.html


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub:
#1864 (comment)


This message was sent using IMP, the Internet Messaging Program.

@davidlange6
Copy link
Contributor

Thanks for the pointer… didn't see that thread (though I notice that L1 had signed off on pre12…)

On Feb 3, 2014, at 2:33 PM, perrotta notifications@github.com wrote:

I agree it is an issue and it has to be followed up.

What I said is simply that it starts at L1 see also

https://hypernews.cern.ch/HyperNews/CMS/get/trigger-performance/705/6.html

and the plots linked therein, for instance

https://cmsweb.cern.ch/dqm/relval/start?runnr=1;dataset=/RelValZMM_13/CMSSW_7_0_0_pre12-POSTLS170_V1-v1/DQM;sampletype=offline_data;filter=all;referencepos=overlay;referenceshow=all;referenceobj1=other%3A%3A/RelValZMM_13/CMSSW_7_0_0_pre11-POSTLS162_V4-v1/DQM%3A;referenceobj2=none;referenceobj3=none;referenceobj4=none;search=;striptype=object;stripruns=;stripaxis=run;stripomit=none;workspace=Everything;size=M;root=HLT/Muon/Efficiency_Layouts/HLT_Mu5;focus=;zoom=no;

Thus the culprit cannot be L2 muon code, and in particular cannot be
an effect of this PR.

davidlange6 notifications@github.com ha scritto:

is there a thread you know of confirming that? This one just says it
could be a problem….

On Feb 3, 2014, at 2:15 PM, perrotta notifications@github.com
wrote:

Hi David.
The issue you are referencing is known to be due to the L1 stuffs
in the DT, it has nothing to do with the L2/L3 HLT muons:
https://hypernews.cern.ch/HyperNews/CMS/get/muon-object-validation/499/2.html


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub:
#1864 (comment)


This message was sent using IMP, the Internet Messaging Program.

Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants