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

update trajectory filters (minNumberOfHits --> minNumberOfHitsForLoopers) #13421

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Feb 22, 2016

this PR is meant to update the name of a trajectory filter parameter (minNumberOfHits --> minNumberOfHitsForLoopers)

I also take the opportunity of deleting few existAs checks which were still present in the TrackingTools/TrajectoryFiltering package
=> the HLT customization function (HLTrigger/Configuration/python/customizeHLTforCMSSW.py) has been updated in order to make the HLT menu compatible w/ this update ("usual" temporary solution waiting for the confDB update)

in addition, I noticed there were few cases in which the class constructor, where default input parameters were used, was not in synch w/ the default parameter values given by config
=> the default parameters have been updated as well

I ran
HLTrigger/Configuration/test/cmsDriver.sh
and
addOnTests.py
I got the few errors ([1] from cmsDriver and [2] from addOnTests, respectively)
but I think they are not related to my changes

@VinInn @rovere @perrotta @Martin-Grunewald

[1]
Creating DigiL1RawHLT HIon_MC
DIGI:pdigi_valid,L1,DIGI2RAW,HLT:HIon,ENDJOB
entry /store/relval/CMSSW_7_6_0_pre6/RelValZEEMM_13_HI/GEN-SIM/76X_mcRun2_HeavyIon_v4-v1/00000/EA469164-6A69-E511-A361-008CFA008768.root
###################################################################

WARNING: this module is deprecated.

Please use CondCore.CondDB.CondDB_cfi.py

###################################################################
Step: DIGI Spec: ['pdigi_valid']
Step: L1 Spec:
L1TCalorimeter Sequence configured for Stage-1 (2015) trigger.
L1TMuon Sequence configured for Legacy trigger (Run1 and Run 2015).
L1TGlobal Sequence configured for Legacy trigger (Run1 and Run 2015).
L1TCalorimeter Conditions configured for Stage-1 (2015) trigger.
Step: DIGI2RAW Spec:
L1TDigiToRaw Sequence configured for Stage-1 (2015) trigger.
Step: HLT Spec: ['HIon']
Step: ENDJOB Spec:

Conditions read from CMS_CONDITIONS via FrontierProd

customising the process with L1THLT from HLTrigger/Configuration/CustomConfigs
customising the process with customizeHLTforFullSim from HLTrigger/Configuration/customizeHLTforMC
Config file RelVal_DigiL1RawHLT_HIon_MC.py created

Creating FastSim HIon_MC
GEN,SIM,RECOBEFMIX,DIGI,L1,L1Reco,RECO,HLT:HIon,ENDJOB
Traceback (most recent call last):
File "/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/cms/cmssw/CMSSW_8_0_X_2016-02-15-2300/bin/slc6_amd64_gcc493/cmsDriver.py", line 55, in
run()
File "/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/cms/cmssw/CMSSW_8_0_X_2016-02-15-2300/bin/slc6_amd64_gcc493/cmsDriver.py", line 11, in run
options = OptionsFromCommandLine()
File "/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/cms/cmssw/CMSSW_8_0_X_2016-02-15-2300/python/Configuration/Applications/cmsDriverOptions.py", line 33, in OptionsFromCommandLine
options=OptionsFromItems(sys.argv[1:])
File "/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/cms/cmssw/CMSSW_8_0_X_2016-02-15-2300/python/Configuration/Applications/cmsDriverOptions.py", line 271, in OptionsFromItems
raise Exception("ERROR: the --option fast is only compatible with the default scenario (--scenario=pp)")
Exception: ERROR: the --option fast is only compatible with the default scenario (--scenario=pp)

[2]
cmsDriver.py TTbar_Tauola_13TeV_cfi -s GEN,SIM,DIGI,L1,DIGI2RAW --mc --scenario=pp -n 10 --conditions auto:run2_mc_GRun --relval 9000,50 --datatier "GEN-SIM-RAW" --eventcontent RAWSIM --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_25ns --magField 38T_PostLS1 --fileout file:RelVal_Raw_GRun_MC.root : FAILED - time: date Wed Feb 17 17:52:54 2016-date Wed Feb 17 17:47:04 2016 s - exit: 34560

log file:
/afs/cern.ch/work/t/tosi/public/trackingHLT/update_trajectory_filtering/CMSSW_8_0_X_2016-02-15-2300/src/HLTrigger/Configuration/test/addOnTests/logs/cmsDriver-hlt_mc_GRun_cmsDriver.py_TTbar_Tauola_13TeV_cfi_-s_GEN,SIM,DIGI,L1,DIGI2RAW_--mc_--scenario=pp_-n_10_--conditions_auto:run2_mc_GRun_--relval_9000,50_--datatier_G.log

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mtosi (mia tosi) for CMSSW_8_0_X.

It involves the following packages:

HLTrigger/Configuration
TrackingTools/TrajectoryFiltering

@perrotta, @cmsbuild, @cvuosalo, @slava77, @Martin-Grunewald, @fwyzard, @davidlange6 can you please review it and eventually sign? Thanks.
@geoff-smith, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @bellan, @jalimena, @cerati, @gpetruc, @istaslis, @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

@VinInn
Copy link
Contributor

VinInn commented Feb 22, 2016

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11434/console

desc.add<edm::ParameterSetDescription>("lostHitsFractionTrajectoryFilter",descLostHitsFraction);
desc.add<edm::ParameterSetDescription>("minHitsTrajectoryFilter", descMinHits);
return desc;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what is this function for? Are we going to get a fillDescriptions() mechanism for global PSets?

If I have understood things correctly, I'm also afraid that the code above would generate a PSet that would not be valid for CkfBaseTrajectoryFilter, that is

cms.PSet(
    looperTrajectoryFilter = cms.PSet(
        ...
    ),
    lostHitsFractionTrajectoryFilter = cms.PSet(
        ...
    ),
    minHitsTrajectoryFilter = cms.PSet(
        ...
    )
)

(although personally I wouldn't mind this style)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ciao
thanks for the remark
such a function is not called yet,
I added it as a book keeper for trying to dealing w/ the fillDescription (as you catch ;) )
yes, it is adding an extra PSet, as now
instead of simply adding the parameters list
(as said this is just a book keeper)

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Feb 22, 2016

@mtosi is this required to be in 80X?
We should probably start with 81X and then backport.
Please make a PR in 81X.

@cvuosalo
Copy link
Contributor

+1

For #13421 91f9442

Code clean-up of trajectory filters. There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2016-02-21-2300 show no significant differences, as expected.

@mtosi
Copy link
Contributor Author

mtosi commented Feb 23, 2016

I've just created PR #13436 (81x)
hope it is fine

@mtosi
Copy link
Contributor Author

mtosi commented Mar 3, 2016

can this PR be integrated ?
thanks

@Martin-Grunewald
Copy link
Contributor

after the l1extra migration!

@mtosi
Copy link
Contributor Author

mtosi commented Mar 3, 2016

right, thanks martin

@mtosi
Copy link
Contributor Author

mtosi commented May 2, 2016

PR #13436 has been closed w/o being merged (81x)
could this PR be integrated ?

@slava77
Copy link
Contributor

slava77 commented May 2, 2016

this PR no longer merges.
Please rebase.

@rovere
Copy link
Contributor

rovere commented May 3, 2016

@Martin-Grunewald was there a problem with this rebase?

@Martin-Grunewald
Copy link
Contributor

80X was just fine and no rebase was needed.
The rebase was needed for the old 81X PR #13436

@rovere
Copy link
Contributor

rovere commented May 3, 2016

I think you are wrong, since while doing the rebase on top of a recent IB we had conflicts...

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2016

@cvuosalo
Copy link
Contributor

cvuosalo commented May 3, 2016

+1

For #13421 b4c0547

Second approval after re-base. Jenkins tests are still OK, but DQM plots show changes from the other PRs Jenkins included in the test.

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@Martin-Grunewald
Copy link
Contributor

@davidlange6
Please integrate this in 80X (the 81X PRs are #13436 / #14317)!

@davidlange6 davidlange6 merged commit b2edca0 into cms-sw:CMSSW_8_0_X May 11, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 12, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 12, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 12, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 13, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 13, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 18, 2016
Martin-Grunewald added a commit to cms-tsg-storm/cmssw that referenced this pull request May 24, 2016
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

9 participants