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
FIX in GBL reference trajectories: add missing initialisations #20262
FIX in GBL reference trajectories: add missing initialisations #20262
Conversation
The code-checks are being triggered in jenkins. |
A new Pull Request was created by @mschrode (Matthias Schroeder) for master. It involves the following packages: Alignment/ReferenceTrajectories @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
+code-checks |
please test |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mschrode Seem to be only minor changes and therefore, I have only the cosmetic comments above.
However, since you use hardcoded numbers (5, 7), you should include them.
PS: Back from parental leave ;)
@@ -603,6 +603,7 @@ namespace gbl { | |||
|
|||
int nOffset = aPoint.getOffset(); | |||
|
|||
for (unsigned int i = 0; i < 5; ++i) anIndex[i] = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -697,6 +698,7 @@ namespace gbl { | |||
|
|||
int nOffset = aPoint.getOffset(); | |||
|
|||
for (unsigned int i = 0; i < 7; ++i) anIndex[i] = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
@@ -1077,13 +1079,15 @@ namespace gbl { | |||
if (measDim > 2) { | |||
matPDer = matP * matDer; | |||
} else { // 'shortcut' for position measurements | |||
matPDer.setZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mschrode please stick to the indentation. This is likely due to tabs in the preceding whitespace.
matPDer.block<2, 5>(3, 0) = matP.block<2, 2>(3, 3) | ||
* matDer.block<2, 5>(3, 0); | ||
} | ||
|
||
if (numInnerTrans > 0) { | ||
// transform for external parameters | ||
proDer.resize(measDim, Eigen::NoChange); | ||
proDer.setZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
@@ -1151,6 +1155,7 @@ namespace gbl { | |||
if (numInnerTrans > 0) { | |||
// transform for external parameters | |||
proDer.resize(nDim, Eigen::NoChange); | |||
proDer.setZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
-1 Tested at: 3eb9476 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: AddOn
I found errors in the following addon tests: cmsDriver.py RelVal -s L1REPACK:Full --data --scenario=pp -n 10 --conditions auto:run2_hlt_PIon --relval 9000,50 --datatier "RAW" --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2017 --eventcontent RAW --fileout file:RelVal_Raw_PIon_DATA.root --filein /store/data/Run2017A/HLTPhysics4/RAW/v1/000/295/606/00000/36DE5E0A-3645-E711-8FA1-02163E01A43B.root : FAILED - time: date Tue Sep 19 17:10:39 2017-date Tue Sep 19 17:07:26 2017 s - exit: 23552 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
failure seems unrelated |
please test |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
Adds missing matrix initialisations to the EIGEN matrices.
This was not needed in previous implementation of GBL based on ROOT TMatrix
(ROOT presets matrices to 0., EIGEN not).
Fix provided by @ckleinw