Phase2-hgx176 Fix bug for HGCal corners #25525
Conversation
|
The code-checks are being triggered in jenkins. |
|
@cmsbuild Please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25525/7726 |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @bsunanda for master. It involves the following packages: Geometry/HGCalGeometry @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@bsunanda is this expected to introduce any differences in the comparisons? |
|
@kpedro88 I don't think so. It only changes the corners which are used only in visualization (I believe). These are indeed bug fixes. Earlier corner calculation was wrong as pointed out by Alex |
|
+upgrade |
|
the added test configuration runs smoothly |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| co[i] = m_cellVec[cellIndex].getPosition(lcoord); | ||
| float dx = k_fac2*m_cellVec[cellIndex].param()[FlatHexagon::k_r]; | ||
| float dy = k_fac1*m_cellVec[cellIndex].param()[FlatHexagon::k_R]; | ||
| float dz =-id.zSide*m_cellVec[cellIndex].param()[FlatHexagon::k_dZ]; |
There was a problem hiding this comment.
It looks like dx, dy, and dz are not changed after initialization. It would be good to make them const. Same for lines 303-305.
There was a problem hiding this comment.
It is an incorrect judgement. Though dx, dy, dz may be the same for a large number of cells, they will be different for all cells. We have so called fine/coarse cells and also with different depletion thickness. Please sign this PR
There was a problem hiding this comment.
I was merely suggesting changing, for example, line 316 from:
float dx = k_fac2*m_cellVec[cellIndex].param()[FlatHexagon::k_r];
to
const float dx = k_fac2*m_cellVec[cellIndex].param()[FlatHexagon::k_r];
but not moving or otherwise changing the definition of dx. dx from line 316 is read once and then goes out of scope. It is not changed, so it is a constant.
Our CMS coding guidelines say that all variables should be declared const unless they need to be changed (Rule 7.5). The coding guidelines are still under review right now, so it would be good for all developers to take a look at them to see if any changes are needed.
It's a very minor issue. The const declaration would just help someone reading the code understand that dx, dy, and dz don't change after being initialized.
|
@bsunanda please respond to the code review |
|
@bsunanda when do you expect an update of this PR? |
|
I do not think we need further updates except a sign-off by Carl. He misunderstood some of the code. |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
geometry-approved
and removed
geometry-pending
pending-signatures
labels
|
code-checks |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25525/8150
|
|
+1 |
Uses the corner calculation of new HGCal cells