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

Various fixes related to HCAL OOT pileup corrections code #3909

Merged
merged 2 commits into from May 20, 2014

Conversation

igv4321
Copy link
Contributor

@igv4321 igv4321 commented May 16, 2014

Moved BoostIODBWriter.h and BoostIODBReader.h from
RecoLocalCalo/HcalRecAlgos/interface to CondTools/Hcal/interface
(per request from Slava and suggestion from Salvatore Di Guida).

Added a convenience config cfi CondDBboost_cfi.py in which
dbFormat is set to boost I/O.

Moved OOT pileup database interface plugin from the "plugins"
directory of RecoLocalCalo/HcalRecAlgos into "test" directory.

Added "data" directory in RecoLocalCalo/HcalRecAlgos and
placed a database file with current OOT pileup corrections objects
there.

No changes expected in functionality or tests.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @igv4321 for CMSSW_7_1_X.

Various fixes related to HCAL OOT pileup corrections code

It involves the following packages:

CondCore/CondDB
CondTools/Hcal
RecoLocalCalo/HcalRecAlgos

@apfeiffer1, @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @ggovi, @Degano can you please review it and eventually sign? Thanks.
@ghellwig, @argiro 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented May 18, 2014

why not move the rest of DBWriter/Reader that are still in RecoLocalCalo/HcalRecAlgos/ to CondTools/Hcal
?

@igv4321
Copy link
Contributor Author

igv4321 commented May 18, 2014

Because the template parameters for which instantiations are performed
implement hcal reconstruction algorithms

On 05/18/2014 10:20 AM, Slava Krutelyov wrote:

why not move the rest of DBWriter/Reader that are still in RecoLocalCalo/HcalRecAlgos/ to CondTools/Hcal
?


Reply to this email directly or view it on GitHub #3909 (comment).

@slava77
Copy link
Contributor

slava77 commented May 18, 2014

I'm not sure I follow:
RecoLocalCalo/HcalRecAlgos/test/OOTPileupCorrectionDBModules.cc
and the reader/writer py files depend only on CondTools and CondFormats (they don't include anything from RecoLocalCalo)

... the data/ files here could also be in the Cond* area as well (as they are just temporary replacements of what's supposed to be in the database anyways).

@igv4321
Copy link
Contributor Author

igv4321 commented May 18, 2014

Yes, this is because part of the Hcal reconstruction code now leaves
in CondFormats (in particular, OOT pileup correction objects).
This doesn't mean that all of the code related to OOT pileup corrections
should be placed there, including the code that does not represent storable
objects.

On 05/18/2014 10:35 AM, Slava Krutelyov wrote:

I'm not sure I follow:
RecoLocalCalo/HcalRecAlgos/test/OOTPileupCorrectionDBModules.cc
and the reader/writer py files depend only on CondTools and CondFormats (they don't include anything from RecoLocalCalo)

... the data/ files here could also be in the Cond* area as well (as they are just temporary replacements of what's supposed to be in the database anyways).


Reply to this email directly or view it on GitHub #3909 (comment).

@slava77
Copy link
Contributor

slava77 commented May 18, 2014

The functionality of OOT_DBWriter and Reader are just to operate on conditions objects,
similar to this case, other items in CondTools/Hcal depend of CondFormats/HcalObjects and don't get scattered to other places especially in this way (the header is in CondTools, the instance is in RecoLocalCalo).
So, OOT_DBWriter and Reader should be in CondTools as well.
What should be in the RecoLocalCalo/HcalRecAlgos is just the code that reads calo objects (hits etc) from the events and computes some derived quantities.

Code that reads conditions and converts them to some other format of conditions just doesn't belong in reco.

@davidlange6
Copy link
Contributor

I agree w/ Slava, indeed

@cmsbuild
Copy link
Contributor

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

@igv4321
Copy link
Contributor Author

igv4321 commented May 18, 2014

OK, moved

On 05/18/2014 01:46 PM, David Lange wrote:

I agree w/ Slava, indeed


Reply to this email directly or view it on GitHub #3909 (comment).

@cmsbuild
Copy link
Contributor

-1
When I ran the RelVals I found an error in the following worklfows:
4.22 step2

runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented May 19, 2014

+1

for #3909 d0b9b34
jenkins OK
tested in CMSSW_7_1_X_2014-05-16-0200 (test are sign381)
no diffs, as expected.

@apfeiffer1
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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

ktf added a commit that referenced this pull request May 20, 2014
Reco -- Various fixes related to HCAL OOT pileup corrections code
@ktf ktf merged commit 4c57be0 into cms-sw:CMSSW_7_1_X May 20, 2014
@igv4321 igv4321 deleted the oot-pileup-code-update branch September 12, 2014 02:51
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

7 participants