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

Adding general track to ALCARECO for muonalign #15221

Merged
merged 5 commits into from Aug 23, 2016

Conversation

lpernie
Copy link
Contributor

@lpernie lpernie commented Jul 18, 2016

Hi @mmusich,
This the the new PR on the addition of general tracks to the ALCARECO stream for muon alignment you requested.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lpernie (Luca) for CMSSW_8_1_X.

It involves the following packages:

Alignment/CommonAlignmentProducer
Configuration/PyReleaseValidation
Configuration/StandardSequences

@ghellwig, @franzoni, @cerminar, @fabozzi, @cmsbuild, @srimanob, @mmusich, @hengne, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @pakhotin, @makortel, @GiacomoSguazzoni, @tocheng, @VinInn, @Martin-Grunewald, @rovere, @tlampen, @mschrode, @mmusich, @dgulhan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@lpernie
Copy link
Contributor Author

lpernie commented Jul 18, 2016

Should I rebase it?
git fetch official-cmssw # get the latest updates from the official repository
git checkout newALCARECOstream # checkout your branch (if you have not done yet)
git rebase official-cmssw/CMSSW_8_1_X # rebase your changes on top of the official branch

@mmusich
Copy link
Contributor

mmusich commented Jul 19, 2016

@lpernie, yes, this needs a rebase, but be careful that in your current version there are regressions in Configuration/StandardSequences/python/AlCaRecoStreams_cff.py, namely you shouldn't remove ALCARECOStreamHcalCalIsolatedBunchFilter

@cmsbuild
Copy link
Contributor

Pull request #15221 was updated. @ghellwig, @franzoni, @cerminar, @fabozzi, @cmsbuild, @srimanob, @mmusich, @hengne, @davidlange6 can you please check and sign again.

@lpernie
Copy link
Contributor Author

lpernie commented Jul 19, 2016

Done!
(I saw that Configuration/StandardSequences/python/AlCaRecoStreams_cff.py had no conflicts)

@mmusich
Copy link
Contributor

mmusich commented Jul 19, 2016

@lpernie,
as far as I can see the regressions are still there.
Configuration/StandardSequences/python/AlCaRecoStreams_cff.py, namely, you are still removing:

 -ALCARECOStreamHcalCalIsolatedBunchFilter = cms.FilteredStream(
-   responsible = 'Sunanda Banerjee',
-   name = 'HcalCalIsolatedBunchFilter',
-   paths  = (pathALCARECOHcalCalIsolatedBunchFilter),
-   content = OutALCARECOHcalCalIsolatedBunchFilter.outputCommands,
-   selectEvents = OutALCARECOHcalCalIsolatedBunchFilter.SelectEvents,
-   dataTier = cms.untracked.string('ALCARECO')
-   )
-

can you please get it back?
Thanks,

Marco

@cmsbuild
Copy link
Contributor

Pull request #15221 was updated. @ghellwig, @franzoni, @cerminar, @fabozzi, @cmsbuild, @srimanob, @mmusich, @hengne, @davidlange6 can you please check and sign again.

@@ -75,9 +75,6 @@
from Calibration.HcalAlCaRecoProducers.ALCARECOHcalCalNoise_cff import *
#HCAL calibration iterative PhiSym
from Calibration.HcalAlCaRecoProducers.ALCARECOHcalCalIterativePhiSym_cff import *
# HCAL isolated bunch
from Calibration.HcalAlCaRecoProducers.ALCARECOHcalCalIsolatedBunchFilter_cff import *

Copy link
Contributor

Choose a reason for hiding this comment

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

@lpernie also this needs to get back

@lpernie
Copy link
Contributor Author

lpernie commented Jul 19, 2016

Ok, I just added it back. I hope I did correctly.

@@ -170,10 +167,12 @@
pathALCARECOHcalCalIsoTrkFilter = cms.Path(seqALCARECOHcalCalIsoTrkFilter)
pathALCARECOHcalCalNoise = cms.Path(seqALCARECOHcalCalNoise)
pathALCARECOHcalCalIterativePhiSym = cms.Path(seqALCARECOHcalCalIterativePhiSym*ALCARECOHcalCalPhisymDQM)
pathALCARECOHcalCalIsolatedBunchFilter = cms.Path(seqALCARECOHcalCalIsolatedBunchFilter)
Copy link
Contributor

Choose a reason for hiding this comment

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

@lpernie this one as well!

@cmsbuild
Copy link
Contributor

Pull request #15221 was updated. @ghellwig, @franzoni, @cerminar, @fabozzi, @cmsbuild, @srimanob, @mmusich, @hengne, @davidlange6 can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Jul 19, 2016

thanks! should be in a testable shape now

@mmusich
Copy link
Contributor

mmusich commented Jul 19, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 19, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@ghellwig
Copy link

ghellwig commented Aug 22, 2016

Hi @lpernie, @mmusich, @franzoni,

I have run an additional check regarding the change of the ALCARECO troughput at commit c03b999:

I modified workflow 1000, to take SingleMuon and DoubleMuon datasets as input and compared the file sizes of the produced output of the MuAlCalIsolatedMu and the MuAlZMuMu streams to get a measure of the expected change in ALCARECO disk usage:

Used input events: 500

Selected events (before #15221):

  • SingleMuon:
    • MuAlZMuMu.root: 18
    • MuAlCalIsolatedMu.root: 490
  • DoubleMuon:
    • MuAlZMuMu.root: 30
    • MuAlCalIsolatedMu.root: 478

Selected events (after #15221):

  • SingleMuon:
    • MuAlZMuMu.root: 18
    • MuAlCalIsolatedMu.root: 480
  • DoubleMuon:
    • MuAlZMuMu.root: 31
    • MuAlCalIsolatedMu.root: 403

Event size (before #15221):

  • SingleMuon:
    • MuAlZMuMu.root: 162295 bytes/event
    • MuAlCalIsolatedMu.root: 38866 bytes/event
  • DoubleMuon:
    • MuAlZMuMu.root: 114372 bytes/event
    • MuAlCalIsolatedMu.root: 41461 bytes/event

Event size (after #15221):

  • SingleMuon:
    • MuAlZMuMu.root: 168392 bytes/event
    • MuAlCalIsolatedMu.root: 42145 bytes/event
  • DoubleMuon:
    • MuAlZMuMu.root: 118663 bytes/event
    • MuAlCalIsolatedMu.root: 45909 bytes/event

relative throughput change = (file_size(after) - file_size(before))/file_size(before)

  • SingleMuon:
    • MuAlZMuMu.root: (3031048 - 2921303) / 2921303 = 3.8%
    • MuAlCalIsolatedMu.root: (20229479 - 19044339) / 19044339 = 6.2%
  • DoubleMuon:
    • MuAlZMuMu.root: (3678552 - 3431168) / 3431168 = 7.2%
    • MuAlCalIsolatedMu.root: (18501503 - 19818406) / 19818406 = -6.6%

Hope this helps to judge this PR in terms of ALCARECO throughput.

-Gregor

@ghellwig
Copy link

To be more specific: I used the following datasets:

@franzoni
Copy link

+1
with further removal of access to RECO some level of extra alcareco resources is inevitable; this will also need to reflect on the overall disk provision to alcs/db at tier 2's in general and at CAF

thanks @ghellwig for the study

@mmusich
Copy link
Contributor

mmusich commented Aug 22, 2016

@fabozzi @srimanob @hengne, can you please cross-check and sign? Thanks
M.

@mmusich
Copy link
Contributor

mmusich commented Aug 22, 2016

@lpernie in view of a possible reprocessing of 2016 data can you please backport to 80x as well?

@lpernie
Copy link
Contributor Author

lpernie commented Aug 22, 2016

Sure.
This mean should I make a PR with the same changes in 80?

@mmusich
Copy link
Contributor

mmusich commented Aug 22, 2016

Indeed. Thanks!

@fabozzi
Copy link
Contributor

fabozzi commented Aug 23, 2016

+1

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 90a07cb into cms-sw:CMSSW_8_1_X Aug 23, 2016
steps['RECOCOS']=merge([{'-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlCalIsolatedMu,DQM','--scenario':'cosmics'},stCond,step3Defaults])
steps['RECOHAL']=merge([{'-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlCalIsolatedMu,DQM','--scenario':'cosmics'},step3Up2015Hal])
steps['RECOCOS']=merge([{'-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlGlobalCosmics,DQM','--scenario':'cosmics'},stCond,step3Defaults])
steps['RECOHAL']=merge([{'-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlBeamHalo+MuAlBeamHaloOverlaps,DQM','--scenario':'cosmics'},step3Up2015Hal])
steps['RECOCOS_UP15']=merge([{'--conditions':'auto:run2_mc_cosmics','-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlCalIsolatedMu,DQM','--scenario':'cosmics'},step3Up2015Hal])
Copy link
Contributor

Choose a reason for hiding this comment

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

@lpernie this line here shall be changed as done above:
https://github.com/cms-sw/cmssw/pull/15221/files#diff-819af2ec677272fddf1fef50e788a88bR1195"
this currently breaks wf 1307.0 @MengqingWu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmusich Do you mean like RECOHAL?
steps['RECOCOS']=merge([{'-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlBeamHalo+MuAlBeamHaloOverlaps,DQM','--scenario':'cosmics'},stCond,step3Defaults])
Or like it was in the past (ALCA:MuAlCalIsolatedMu)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lpernie no I mean to change:

steps['RECOCOS_UP15']=merge([{'--conditions':'auto:run2_mc_cosmics','-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlCalIsolatedMu,DQM','--scenario':'cosmics'},step3Up2015Hal]) 

to:

  steps['RECOCOS_UP15']=merge([{'--conditions':'auto:run2_mc_cosmics','-s':'RAW2DIGI,L1Reco,RECO,ALCA:MuAlGlobalCosmics,DQM','--scenario':'cosmics'},step3Up2015Hal]) 

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