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

V2 of string-based configuration of ecal channel status with HLT changes #3735

Merged
merged 11 commits into from May 9, 2014

Conversation

Martin-Grunewald
Copy link
Contributor

Channel statuses are with this PR configurable using symbolic names rather than numbers. No impact on physics - contains #3448 and replaces #3682 including cosmics fix.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

A new Pull Request was created by @Martin-Grunewald (Martin Grunewald) for CMSSW_7_1_X.

V2 of string-based configuration of ecal channel status with HLT changes

It involves the following packages:

CondFormats/EcalObjects
DataFormats/EcalRecHit
HLTrigger/Configuration
RecoLocalCalo/Configuration
RecoLocalCalo/EcalRecAlgos
RecoLocalCalo/EcalRecProducers

@perrotta, @cmsbuild, @apfeiffer1, @diguida, @fwyzard, @Martin-Grunewald, @anton-a, @thspeer, @rcastello, @slava77, @ggovi, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
@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.

@Martin-Grunewald
Copy link
Contributor Author

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2014

@diguida
Copy link
Contributor

diguida commented May 8, 2014

+1
the change in the EcalChannelStatusCode are not impacting neither DB serialisation (static const data members manipulated), nor AlCa client code.

@argiro
Copy link
Contributor

argiro commented May 9, 2014

I’ll try to understand what’s going on today

On 08 May 2014, at 11:36, Slava Krutelyov notifications@github.com wrote:

@argiro

Hi Stefano, please comment on the regressions observed in fastsim and HLT (notes in #3682)


Reply to this email directly or view it on GitHub.

@argiro
Copy link
Contributor

argiro commented May 9, 2014

Slava, I fail to catch differences in configuration of fast sim (hlt_famos) and HLT before and after the merge.
From your report I understand that in data and Full simulation those discrepancies are not observed, right ?
Ecal reconstruction in sim/fastsim/data uses the same configuration if I am not mistaken.
I’ll need to dig deeper.

Stefano

On 09 May 2014, at 08:45, Stefano Argiro' stefano.argiro@cern.ch wrote:

I’ll try to understand what’s going on today

On 08 May 2014, at 11:36, Slava Krutelyov notifications@github.com wrote:

@argiro

Hi Stefano, please comment on the regressions observed in fastsim and HLT (notes in #3682)


Reply to this email directly or view it on GitHub.

@Martin-Grunewald
Copy link
Contributor Author

Hmm, how do we proceed for making pre8?

Can you sign so that it gets into pre8, given the differences
are small, and so that any correction can come as a separate
PR as soon as ready?

I am asking because this change required a ConfDB template
update which is a pain to roll back (and put back in later)
to allow other HLT changes. In general this episode shows
that in future I should probably wait until RECO has signed off on
a change BEFORE I even start the work on the HLT side, due
to the fact that ConfDB forces to "serialise" HLT changes.

@slava77
Copy link
Contributor

slava77 commented May 9, 2014

+1

for #3735 1e20267
based on tests reported in #3682 (#3682 c60f636)
I checked that this PR runs in CMSSW_7_1_X_2014-05-09-0200, looking just at BH and cosmic wflows (for the rest, the code is unchanged).

ECAL (Stefano) plans to follow up on small discrepancies found in my tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2014

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 9, 2014
HLT -- V2 of string-based configuration of ecal channel status with HLT changes
@ktf ktf merged commit 8cbdbbf into cms-sw:CMSSW_7_1_X May 9, 2014
@Martin-Grunewald Martin-Grunewald deleted the ChangeForEcal2 branch May 12, 2014 04:50
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