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

HCAL TP Geometry changes for plan1, phase2 #17469

Merged
merged 4 commits into from Feb 14, 2017

Conversation

matz-e
Copy link
Contributor

@matz-e matz-e commented Feb 9, 2017

Add some bits to fully activate phase2, and add some placeholders to activate for plan1.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

A new Pull Request was created by @matz-e (Matthias Wolf) for CMSSW_9_0_X.

It involves the following packages:

CalibCalorimetry/CaloTPG
Geometry/CaloTopology
Geometry/HcalCommonData
Geometry/HcalTowerAlgo

@ghellwig, @civanch, @Dr15Jones, @arunhep, @ianna, @mdhildreth, @cmsbuild, @rekovic, @franzoni, @cerminar, @mmusich, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @tocheng this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@rekovic
Copy link
Contributor

rekovic commented Feb 9, 2017

Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17710/console Started: 2017/02/09 10:00
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17711/console Started: 2017/02/09 10:31

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

-1

Tested at: 0572ef0

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

I found follow errors while testing this PR

Failed tests: Build ClangBuild

  • Build:

I found an error when building:

>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CaloOnlineTools/HcalOnlineDb/src/DBlmapWriter.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CaloOnlineTools/HcalOnlineDb/src/HcalChannelIterator.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CaloOnlineTools/HcalOnlineDb/src/HcalDbOmds.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CaloOnlineTools/HcalOnlineDb/src/WriteL1TriggerObjetsXml.cc 
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc: In member function 'unsigned int CaloTPGTranscoderULUT::getOutputLUTSize(const HcalTrigTowerDetId&) const':
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc:207:10: error: expected primary-expression before 'else'
          else if (id.ietaAbs() <= theTopology->lastHERing())
          ^
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc:209:10: error: 'else' without a previous 'if'
          else
          ^

  • Clang:

I found a compilation error while trying to compile with clang:
I used this command:
scram b vclean && scram build -k -j 48 USER_CXXFLAGS='-fsyntax-only' COMPILER='llvm compile'

>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/Geometry/HcalTowerAlgo/test/HcalGeometryDump.cc 
>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/Geometry/HcalTowerAlgo/test/HcalGeometryTester.cc 
Entering library rule at src/CalibCalorimetry/CaloTPG/plugins
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc 
>> Compiling edm plugin /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/plugins/CaloTPGTranscoderULUTs.cc 
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc:207:10: error: expected expression
         else if (id.ietaAbs() <= theTopology->lastHERing())
         ^
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2017-02-08-2300/src/CalibCalorimetry/CaloTPG/src/CaloTPGTranscoderULUT.cc:209:10: error: expected expression
         else
         ^


@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

Pull request #17469 was updated. @ghellwig, @civanch, @Dr15Jones, @arunhep, @ianna, @mdhildreth, @cmsbuild, @rekovic, @franzoni, @cerminar, @mmusich, @mulhearn, @davidlange6 can you please check and sign again.

@rekovic
Copy link
Contributor

rekovic commented Feb 9, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17712/console Started: 2017/02/09 11:03

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #17469 was updated. @ghellwig, @civanch, @Dr15Jones, @arunhep, @ianna, @mdhildreth, @cmsbuild, @rekovic, @franzoni, @cerminar, @mmusich, @mulhearn, @davidlange6 can you please check and sign again.

@ianna
Copy link
Contributor

ianna commented Feb 10, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 10, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17731/console Started: 2017/02/10 14:53

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@ianna
Copy link
Contributor

ianna commented Feb 10, 2017

+1

return QIE8_OUTPUT_LUT_SIZE;
else
return QIE10_OUTPUT_LUT_SIZE;
case HcalTopologyMode::TriggerMode_2017plan1:

Choose a reason for hiding this comment

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

hello @matz-e
the case 2017plan1
is the baseline for CMS at this stage: are you planning on implementing it any time soon?
At this moment,

case HcalTopologyMode::TriggerMode_2017plan1:

would proceed to execute the:

case HcalTopologyMode::TriggerMode_2018:

block, which is a worse approximation of TriggerMode_2017plan1 than TriggerMode_2017 is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I plan on getting the implementation done as soon as this is in and we fix the gain widths needed for the LUTs.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @matz-e - for the next iteration - a small lookup table that lets you rewrite all this code as return lutSIze_[id.ietaAbs]; might be worth considering.

@franzoni
Copy link

+1

@davidlange6 davidlange6 merged commit 763413e into cms-sw:CMSSW_9_0_X Feb 14, 2017
@davidlange6
Copy link
Contributor

Seems that there is not - will revert for now - just remake this PR with a fix. Thanks

@matz-e
Copy link
Contributor Author

matz-e commented Feb 16, 2017

Sorry, just got in. I'll have a look at a fix and will remake it.

@matz-e
Copy link
Contributor Author

matz-e commented Feb 16, 2017

@davidlange6 This is due to the geometry being used from the DB, and me redefining the enum values. I'll fix that with my other open PR, by keeping the meaning of the integers in the enum constant, and adjusting labels.

@matz-e matz-e mentioned this pull request Feb 16, 2017
@matz-e matz-e deleted the hcal-tp-geometry-plan1-phase2 branch September 15, 2017 15:01
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

6 participants