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

New Hcal Object - HcalQIEType #12473

Merged
merged 5 commits into from
Dec 2, 2015
Merged

Conversation

walterjr
Copy link
Contributor

First step to create a new object on the Data Base.

This PR only create the object (or class) for the next step (implementation on the code to create tags on the DB using the new object).

This DB container will provide per-HCAL-readout flag of the FE QIE electronics type for choosing a corresponding ADC coder in DIGI/RECO.

It involves the following packages:
CondFormats/DataRecord
CondFormats/HcalObjects

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @walterjr (Walter Alda) for CMSSW_8_0_X.

It involves the following packages:

CondFormats/DataRecord
CondFormats/HcalObjects

@diguida, @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@apfeiffer1 this is something you requested to watch as well.
@Degano you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@abdoulline
Copy link

@diguida, @cerminar, @franzoni, @ggovi, @mmusich, @davidlange6

we need it quickly (=asap) revised/merged to proceed with HCAL HF (dual) readout DIGI/RECO updates for 2016...

@mmusich
Copy link
Contributor

mmusich commented Nov 18, 2015

please test

@cmsbuild
Copy link
Contributor

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

@mmusich
Copy link
Contributor

mmusich commented Nov 18, 2015

@walterjr @abdoulline you need to edit CondCore/Utilities/src/CondDBFetch.cc and CondCore/Utilities/src/CondDBImport.cc in order for the conddb_import tool to fetch and import the class HcalQIEType. This is needed for the dropbox to properly handle the payloads of this type.
You can have a look at the change needed here: https://github.com/cms-sw/cmssw/pull/9105/files

@abdoulline
Copy link

Walter, please modify your branch the way Marco has asked:

CondCore/Utilities/src/CondDBFetch.cc
CondCore/Utilities/src/CondDBImport.cc
CondCore/Utilities/src/CondFormats.h

to add HcalQIETypes (the same way - for instance - as HcalRecoParams is added).
as in https://github.com/cms-sw/cmssw/pull/9105/files

https://github.com/mmusich/cmssw/blob/542a06584a0546f63dd9a5c2635e800dc258aa06/CondCore/Utilities/src/CondDBFetch.cc#L155

https://github.com/mmusich/cmssw/blob/542a06584a0546f63dd9a5c2635e800dc258aa06/CondCore/Utilities/src/CondDBImport.cc#L170

https://github.com/mmusich/cmssw/blob/542a06584a0546f63dd9a5c2635e800dc258aa06/CondCore/Utilities/src/CondFormats.h#L192

@walterjr
Copy link
Contributor Author

@mmusich @abdoulline
At first moment we decide to divide the work in 3+1 steps (modifications+wait):

  1. create the object - current PR
  2. DB issues without change the RelVal test - a preliminary work can be found where [1](including the modifications that you mention however will be edited for final version). Tests has been performed and it is working.
  3. wait for the DB update the code
  4. change the code to run the RelVal test - also on [1] however is necessary to remove some comments in some lines

For now is not necessary to change the CondCore/Utilities files because Step (1) do not take any "action" using the DB. All necessary changes to handle with the DB will be taken on Step (2).

[1] CMSSW_8_0_X...walterjr:NewHcalQIETypeNoRelVal

@abdoulline
Copy link

Walter,

(let Marco correct me, if I'm wrong) -
my understanding is that during the first current 1) step, DB people need
to modify DB to accomodate for the HcalQIETypes, so that one could
upload HcalQIETypes to DB after it.
And for uploading to DB you need to move mentioned 3 files changes from
your (prepared, next) step 2) to this one 1).

BTW, I presume you mean not RelVal, but runTheMatrix test in your 2), 4).

S.

On Wed, 18 Nov 2015, Walter Alda wrote:

@mmusich @abdoulline
At first moment we decide to divide the work in 3+1 steps (modifications+wait):

  1. create the object - current PR
  2. DB issues without change the RelVal test - a preliminary work can be found where 1.
    Tests has been performed and it is working.
  3. wait for the DB update the code
  4. change the code to run the RelVal test - also on [1] however is necessary to remove
    some comments in some lines

For now is not necessary to change the CondCore/Utilities files because Step (1) do not
take any "action" using the DB. All necessary changes to handle with the DB will be
taken on Step (2).

[1] CMSSW_8_0_X...walterjr:NewHcalQIETypeNoRelVal


Reply to this email directly or view it on
GitHub.[AEx02lKJZun2J44E3Q-OhhuPTRYUZ1pKks5pHHl3gaJpZM4GkoIW.gif]

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@abdoulline
Copy link

Walter,
why can't you merge 1) + 2)
I mean to add 2) to this branch?
Do you see any problem, have any objections?

So that all the code changes would be submitted (without breaking runTheMatrix tests) at once?
Then DB experts can modify DB to accommodate for actual payloads.
It would save time which would be needed for sequential approval of two PRs...

@walterjr
Copy link
Contributor Author

Salavat,

Local test with (1)+(2) shows that the runTheMatrix test do not break.
Also is possible to create sqlite files using the new object and in addition also list and dump from the sqlite file is available.
If everybody agree I can update the branch and approval all in a single PR.

@abdoulline
Copy link

Walter,

Once (1)+(2) will be approved/merged, DB experts can proceed with preparing new table in DB for payload.
Then we will be able to upload whatever will be available (via Dropbox).
Please, update the branch.

@cmsbuild
Copy link
Contributor

Pull request #12473 was updated. @diguida, @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please check and sign again.

@diguida
Copy link
Contributor

diguida commented Nov 24, 2015

please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@diguida
Copy link
Contributor

diguida commented Nov 24, 2015

+1

  • correct definition of a new condition object and of the template specialisation for the HCAL general container
  • definition of a record associated to the object
  • activation of serialisation unit test
  • unit tests for reading with ESSource and write via PopCon
  • utilities for importing payloads.

@cmsbuild
Copy link
Contributor

@mmusich
Copy link
Contributor

mmusich commented Nov 24, 2015

ping @ggovi

@abdoulline
Copy link

@ggovi - could you, please, approve it...

@abdoulline
Copy link

@ggovi - could you, please, approve it ?
"Ceterum censeo..."(c)

@ggovi
Copy link
Contributor

ggovi commented Nov 30, 2015

+1

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Dec 2, 2015
@cmsbuild cmsbuild merged commit 188a7f5 into cms-sw:CMSSW_8_0_X Dec 2, 2015
@kpedro88 kpedro88 mentioned this pull request Feb 9, 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

7 participants