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

Rechit #1578

Merged
merged 18 commits into from Dec 3, 2013
Merged

Rechit #1578

merged 18 commits into from Dec 3, 2013

Conversation

aburgm
Copy link
Contributor

@aburgm aburgm commented Nov 25, 2013

Import from old CVS HEAD:

  • Refactor the code
  • Add RecHit Embedding
  • Make it run in CMSSW_7_0_x

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @aburgm (Armin Burgmeier) for CMSSW_7_0_X.

Rechit

It involves the following packages:

GeneratorInterface/ExternalDecays
TauAnalysis/MCEmbeddingTools

@vciulli, @civanch, @nclopezo, @mdhildreth, @cmsbuild, @giamman can you please review it and eventually sign? Thanks.
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.

@cmsbuild
Copy link
Contributor

-1
I found an error when building:

>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/ElectronSeedTrackRefUpdaterAndMerger.cc 
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CaloRecHitMixer.cc:14:2: warning: #warning "ZDCRHMixer still needs to be done" [-Wcpp]
 #warning "ZDCRHMixer still needs to be done" 
  ^
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CandViewCountEventSelFlagProducer.cc: In constructor 'CandViewCountEventSelFlagProducer::CandViewCountEventSelFlagProducer(const edm::ParameterSet&)':
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CandViewCountEventSelFlagProducer.cc:6:23: error: no matching function for call to 'ObjectCountEventSelectoredm::View::ObjectCountEventSelector(const edm::ParameterSet&)'
   : eventSelector_(cfg)
                       ^
/build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CandViewCountEventSelFlagProducer.cc:6:23: note: candidates are:
In file included from /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CandViewCountEventSelFlagProducer.h:23:0,
                 from /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-26-0200/src/TauAnalysis/MCEmbeddingTools/plugins/CandViewCountEventSelFlagProducer.cc:1:


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/1473/summary.html

@ktf
Copy link
Contributor

ktf commented Nov 26, 2013

Please drop the #warning and at least compile your code before doing a pull request.

@cmsbuild
Copy link
Contributor

@aburgm
Copy link
Contributor Author

aburgm commented Nov 26, 2013

I have developed this on top of CMSSW_7_0_0_pre8, where it compiles and runs nicely. I have merged this now with the latest IB and fixed the compiler error and also the #warning.

@ktf
Copy link
Contributor

ktf commented Nov 26, 2013

Please make sure you try it out in the IB as well, since that's where it eventually will end up.

@aburgm
Copy link
Contributor Author

aburgm commented Nov 26, 2013

It compiles and runs in CMSSW_7_0_X_2013-11-26-0200.

@cmsbuild
Copy link
Contributor

@mulhearn
Copy link
Contributor

+1

1 similar comment
@Martin-Grunewald
Copy link
Contributor

+1

@vadler
Copy link

vadler commented Nov 26, 2013

-1
Please clear the warning

****WARNING: Invalid tool EGamma/EGammaAnalysisTools. [...]

This points to a package originally hosted in UserCode. Its CMSSW version EgammaAnalysis/ElectronTools is requested for 70X in #1453 , but not integrated yet.

@jpavel : Are the heavy changes to TauAnalysis/MCEmbeddingTools fine for the Tau POG?

@jpavel
Copy link
Contributor

jpavel commented Nov 26, 2013

Yes, these changes are ok for us (it is just update of the old code we used in 53x and port to 70x). There might be some hypothetical interference with our latest developments, which we would like to push to 70x as well, so I will check it

@aburgm
Copy link
Contributor Author

aburgm commented Nov 26, 2013

I have fixed the warning (simply removing the package from BuildFile.xml, it is not used at the moment).

@cmsbuild
Copy link
Contributor

@perrotta
Copy link
Contributor

+1
Indeed, once rebased the only modified packages wrt 70X IB are
GeneratorInterface/ExternalDecays
TauAnalysis/MCEmbeddingTools
and none of them belongs to HLT

@slava77
Copy link
Contributor

slava77 commented Nov 26, 2013

@aburgm

Hi Armin
git still shows a million diffs. this should be rebased to something more fresh, as already suggested.
It's worth looking at the https://github.com/cms-sw/cmssw/pull/1578/files every time a pull request is submitted (or updated)

Cheers

Slava

@aburgm
Copy link
Contributor Author

aburgm commented Nov 26, 2013

I didn't rebase because then some of the intermediate commits wouldn't compile anymore. However, it can easily be done without conflicts. What is the suggested procedure? Should I open a new pull request, or rewrite the history of this branch?

@ktf
Copy link
Contributor

ktf commented Nov 26, 2013

I would use:

git rebase -i $CMSSW_VERSION

and squash all the intermediate commits that don't compile with their fix.

@cmsbuild
Copy link
Contributor

Pull request #1578 was updated. @vciulli, @civanch, @nclopezo, @mdhildreth, @cmsbuild, @giamman can you please check and sign again.

@aburgm
Copy link
Contributor Author

aburgm commented Nov 26, 2013

Rebased to CMSSW_7_0_X_2013-11-26-0200

@vadler
Copy link

vadler commented Nov 26, 2013

+1
Nevertheless should TauAnalysis/MCEmbeddingTools/test/INSTRUCTIONS be fixed, so that it runs as is, if you add it to CMSSW.

@vadler
Copy link

vadler commented Nov 26, 2013

Ah, I just realised, that it is not my cup of tea any more ;-)

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Nov 27, 2013

+1

@slava77
Copy link
Contributor

slava77 commented Nov 27, 2013

@ktf
Giulio,
are we really going to let binary (root) files to go to the release?

@ktf
Copy link
Contributor

ktf commented Nov 27, 2013

No. Please drop it.

@vadler
Copy link

vadler commented Nov 27, 2013

One idea how to deal with binaries, if they are essential for the code to work, is shown here for EgammaAnalysis/ElectronTools/data/ :
https://twiki.cern.ch/twiki/bin/viewauth/CMS/MultivariateElectronIdentification#Recipe_for_53X_from_Git
with
https://github.com/cms-sw/cmssw/blob/CMSSW_5_3_X/EgammaAnalysis/ElectronTools/data/download.url
Maybe not too elegant, but the recipe is used routinely for quite a while already.

@aburgm
Copy link
Contributor Author

aburgm commented Nov 27, 2013

Thanks Volker. Do you know how to copy the data files appear at http://cmsdoc.cern.ch/cms/data/CMSSW/RecoEgamma/ElectronIdentification/data?

Another possibility would be to change the root files (they are basically 2D look-up tables for scale factors) to CSV format, which I see is also used in the ElectronID code.

Also, make it compile in CMSSW_7_0_X_2013-11-26-0200
aburgm and others added 16 commits November 29, 2013 10:12
The default configuration is to run on a pre-skimmed sample. This is
more efficient when having to run over the same dataset many times, for
example when creating embedded samples for many different decay modes.
This allows a configuration file generated with cmsDriver to work out of
the box.
…figuration

This allows to run the code with configurations not used for the Htautau
analysis, for example mdtau=0. In those cases no embeddingKineWeights
are being produced (and a warning message is printed).

However, it is not a fatal issue. Maybe they are not needed, or if they
are they can be computed and applied still after the actual sample
production.
It looks like between CMSSW_5_3_X and CMSSW_7_0_X the decay() function
of the TauolaInterface was changed such that it modifies the event
inplace instead of returning a new event. This commit adapts the code
accordingly.
Basically the pfPileUp and pfNoPileup modules now produce a
std::vector<edm::FwdPtr<reco::PFCandidate> > instead of a
std::vector<reco::PFCandidate>.
which was advanced a bit since CVS HEAD.
ZDC is not relevant for embedded tau samples
The package is actually not needed at the moment.
@aburgm
Copy link
Contributor Author

aburgm commented Nov 29, 2013

I have removed the binary .root file. These are used for correction factors that are not being saved now. This is not dramatic as they can still be computed on top of the generated samples. On a longer term these should maybe go into the DB and then the embedding code can use them from there.

@cmsbuild
Copy link
Contributor

Pull request #1578 was updated. @vciulli, @civanch, @nclopezo, @mdhildreth, @cmsbuild, @giamman can you please check and sign again.

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Nov 29, 2013

+1

1 similar comment
@vciulli
Copy link
Contributor

vciulli commented Dec 3, 2013

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2013

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

nclopezo added a commit that referenced this pull request Dec 3, 2013
@nclopezo nclopezo merged commit 73b00d3 into cms-sw:CMSSW_7_0_X Dec 3, 2013
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
…meter

Update L1Trigger-L1TCalorimeter to V01-00-04
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