Patatrack integration - HCAL local reconstruction (8/N) #31720
Conversation
|
The code-checks are being triggered in jenkins. |
|
For all questions, please address @mariadalfonso @vkhristenko . |
|
-code-checks ERROR: Build errors found during clang-tidy run. |
|
I guess that for HCALxxxGPU conditions (.h and .cc) a natural place would be the same one as for regular conditions: |
See [#31703](cms-sw/cmssw#31703) Patatrack integration - common data formats (5/N) [#31704](cms-sw/cmssw#31704) Patatrack integration - calorimeters shared code (6/N) [#31719](cms-sw/cmssw#31719) Patatrack integration - ECAL local reconstruction (7/N) [#31720](cms-sw/cmssw#31720) Patatrack integration - HCAL local reconstruction (8/N) [#31721](cms-sw/cmssw#31721) Patatrack integration - Pixel local reconstruction (9/N) [#31722](cms-sw/cmssw#31722) Patatrack integration - Pixel track reconstruction (10/N) [#31723](cms-sw/cmssw#31723) Patatrack integration - Pixel vertex reconstruction (11/N)
See - cms-sw/cmssw#31703: Patatrack integration - common data formats (5/N) - cms-sw/cmssw#31704: Patatrack integration - calorimeters shared code (6/N) - cms-sw/cmssw#31719: Patatrack integration - ECAL local reconstruction (7/N) - cms-sw/cmssw#31720: Patatrack integration - HCAL local reconstruction (8/N) - cms-sw/cmssw#31721: Patatrack integration - Pixel local reconstruction (9/N) - cms-sw/cmssw#31722: Patatrack integration - Pixel track reconstruction (10/N) - cms-sw/cmssw#31723: Patatrack integration - Pixel vertex reconstruction (11/N)
| } | ||
|
|
||
| template <> | ||
| constexpr uint32_t compute_nsamples<Flavor5>(uint32_t const nwords) { |
There was a problem hiding this comment.
Why this specialization, since WORDS_PER_SAMPLE = 1 / SAMPLES_PER_WORD ?
There was a problem hiding this comment.
I would say this is to avoid a division of an integer by 0.5, which would involve floating point operations - while multiplying by 2 can be done with integer operations.
There was a problem hiding this comment.
What I meant is: why cannot you use the previous templated compute_nsamples also for Flavor = Flavor5 ? (Then I agree with you that in all cases one could simply multiply by SAMPLES_PER_WORD instead of dividing by WORDS_PER_SAMPLE)
There was a problem hiding this comment.
the other flavors were removed for which WORDS_PER_SAMPLE was 2 (flavor 2), which means you again have floating point in there... i just tried to keep only int operations in there...
There was a problem hiding this comment.
why cannot you use the previous templated
compute_nsamplesalso forFlavor = Flavor5?
The generic version is (lines 89-92)
template <typename Flavor>
constexpr uint32_t compute_nsamples(uint32_t const nwords) {
return (nwords - Flavor::HEADER_WORDS) / Flavor::WORDS_PER_SAMPLE;
}For Flavor5 that would become
constexpr uint32_t compute_nsamples(uint32_t const nwords) {
return (nwords - 1) / 0.5;
}which does not look very sensible.
The Flavor5 specialisation instead becomes
constexpr uint32_t compute_nsamples(uint32_t const nwords) {
return (nwords - 1) * 2;
}which avoids floating point math for an integer multiplication by 2.
Then I agree with you that in all cases one could simply multiply by
SAMPLES_PER_WORDinstead of dividing byWORDS_PER_SAMPLE
I have no idea if - in principle - there can be cases where WORDS_PER_SAMPLE is greater than 1.
If that is not foreseen, then indeed we can change the generic version to
template <typename Flavor>
constexpr uint32_t compute_nsamples(uint32_t const nwords) {
return (nwords - Flavor::HEADER_WORDS) * Flavor::SAMPLES_PER_WORD;
}If we want to keep this more generic, it could become
template <typename Flavor>
constexpr uint32_t compute_nsamples(uint32_t const nwords) {
if constexpr(Flavor::SAMPLES_PER_WORD >= 1)
return (nwords - Flavor::HEADER_WORDS) * Flavor::SAMPLES_PER_WORD;
else
return (nwords - Flavor::HEADER_WORDS) / Flavor::WORDS_PER_SAMPLE;
}|
|
||
| // get to the payload | ||
| auto const* payload64 = buffer + 2 + namc + amcoffset; | ||
| //amcoffset += amcSize; |
There was a problem hiding this comment.
If this line is not needed, it can be removed. Otherwise just add another comment line which explains why do you want to keep here even if commented out
| #ifdef HCAL_RAWDECODE_GPUDEBUG | ||
| // uhtr header v1 1st 64 bits | ||
| auto const payload64_w0 = payload64[0]; | ||
| //uint32_t const data_length64 = payload64_w0 & 0xfffff; |
There was a problem hiding this comment.
If this line is not needed, it can be removed. Otherwise just add another comment line which explains why do you want to keep here even if commented out
|
|
||
| // FIXME: shift should be treated properly, | ||
| // here assume 8 time slices and 8 samples | ||
| auto const shift = 4 - soi; // as in cpu version! |
There was a problem hiding this comment.
Couldn't this "4" be made a named constant?
| if (npassive == 0) | ||
| break; | ||
|
|
||
| //s.head(npassive) |
There was a problem hiding this comment.
Please remove commented out lines, if not needed; or alternatively add another comment line which explains why they must be kept here
| auto const i_real = pulseOffsets(i); | ||
| solution(i_real) += alpha * (s(i) - solution(i_real)); | ||
| } | ||
| //solution.head(npassive) += alpha * |
There was a problem hiding this comment.
Please remove commented out lines, if not needed; or alternatively add another comment line which explains why they must be kept here
| auto const id = gch < nchannelsf01HE | ||
| ? idsf01HE[gch] | ||
| : (gch < nchannelsf015 ? idsf5HB[gch - nchannelsf01HE] : idsf3HB[gch - nchannelsf015]); | ||
| //auto const id = gch >= nchannelsf01HE |
There was a problem hiding this comment.
Please remove commented out lines, if not needed; or alternatively add another comment line which explains why they must be kept here
| #pragma unroll | ||
| for (int i = 0; i < NPULSES; ++i) | ||
| pulseOffsets(i) = i; | ||
| // pulseOffsets(i) = pulseOffsetValues[i] - pulseOffsetValues[0]; |
There was a problem hiding this comment.
Please remove commented out lines, if not needed; or alternatively add another comment line which explains why they must be kept here
That's a valid point, but it requires any one of the following:
Please lt us know which is/are the preferred option/s. |
|
The code-checks are being triggered in jenkins. |
|
-code-checks ERROR: Build errors found during clang-tidy run. |
cmsbuild
added
code-checks-rejected
heterogeneous-pending
and removed
code-checks-pending
new-package-pending
labels
|
The four differences are due to a different number of |
|
For the +%MSG-w XrdAdaptor: MixingModule:mix 03-Dec-2020 09:36:29 CET Run: 1 Event: 1
+Data is served from cern.ch instead of original site eoscms
+%MSGin step 2, and one less -%MSG-w XrdAdaptor: MixingModule:mix 03-Dec-2020 04:14:48 CET Run: 1 Event: 1
-Data is served from cern.ch instead of original site eoscms
-%MSGin step 3. For the 1325.7 workflow, I see some actual differences in the logs. @@ -6,20 +11,11 @@ Step: DQM Spec: ['@nanoAODDQM']
customising the process with nanoAOD_customizeMC from PhysicsTools/NanoAOD/nano_cff
Updating process to run DeepFlavour btag
Will recalculate the following discriminators: pfDeepFlavourJetTags:probb, pfDeepFlavourJetTags:probbb, pfDeepFlavourJetTags:problepb, pfDeepFlavourJetTags:probc
-**************************************************************
-b tagging needs to be run on uncorrected jets. Hence, the JECs
-will first be undone for 'updatedPatJetsWithDeepInfo' and then applied to
-'updatedPatJetsTransientCorrectedWithDeepInfo'.
-**************************************************************
Updating process to run DeepBoostedJet on datasets before 103X
Updating process to run ParticleNet before it's included in MiniAOD
Updating process to run DeepDoubleX on datasets before 104X
Updating process to run DeepDoubleXv2 on datasets before 11X
-Will recalculate the following discriminators on AK8 jets: pfDeepBoostedJetTags:probTbcq, pfDeepBoostedJetTags:probTbqq, pfDeepBoostedJetTags:probTbc, pfDeepBoostedJetTags:probTbq, pfDeepBoostedJetTags:probWcq, pfDeepBoostedJetTags:probWqq, pfDeepBoostedJetTags:probZbb,
+Will recalculate the following discriminators on AK8 jets: pfDeepBoostedJetTags:probTbcq, pfDeepBoostedJetTags:probTbqq,
-add DeepMET Producers
-customising the process with customiseWithTimeMemoryJobReport from Validation/Performance/TimeMemoryJobReport
-Starting cmsRun -j JobReport2.xml step2_NANO_DQM.py
-**************************************************************
pfDeepBoostedJetTags:probTbc, pfDeepBoostedJetTags:probTbq, pfDeepBoostedJetTags:probWcq, pfDeepBoostedJetTags:probWqq, pfDeepBoostedJetTags:probZbb,
b tagging needs to be run on uncorrected jets. Hence, the JECs
will first be undone for 'updatedPatJetsWithDeepInfo' and then applied to
'updatedPatJetsTransientCorrectedWithDeepInfo'.and in step 3 -HARVESTING:@nanoAODDQM
-entry file:step2_inDQM.root
-Step: HARVESTING Spec: ['@nanoAODDQM']
-customising the process with customiseWithTimeMemoryJobReport from Validation/Performance/TimeMemoryJobReport
-Starting cmsRun -j JobReport3.xml step3_HARVESTING.pyI doubt any of them is due to this PR, though. |
|
I have run the comparison scripts in RecoLocalCalo/HcalRecProducers/test, and here are a few CPU-GPU comparison for the reconstructed quantities.
Is there any comment about the M0 distributions? |
|
I brought this up with @mariadalfonso some time ago, who told me (translated from italian)
|
|
One of the corrections is not applied for m0. I think it was cause I was
told it will not be needed at first, then was told that it should be in...
…On Thu, 3 Dec 2020 at 18:39, Andrea Bocci ***@***.***> wrote:
I brought this up with @mariadalfonso <https://github.com/mariadalfonso>
some time ago, who told me (translated from italian)
that's because some parts have not been implemented yet; there are some
TODO left in the code by Viktor
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#31720 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSFUCI7NP3PIYOGIO6UFIDSS7EN5ANCNFSM4SJJVMRA>
.
|
|
To add, that in prs for patatrack it was stated what was not implemented.
This just didn’t propogate here
On Thu, 3 Dec 2020 at 19:12, Viktor Khristenko <vdkhristenko1991@gmail.com>
wrote:
… One of the corrections is not applied for m0. I think it was cause I was
told it will not be needed at first, then was told that it should be in...
On Thu, 3 Dec 2020 at 18:39, Andrea Bocci ***@***.***>
wrote:
> I brought this up with @mariadalfonso <https://github.com/mariadalfonso>
> some time ago, who told me (translated from italian)
>
> that's because some parts have not been implemented yet; there are some
> TODO left in the code by Viktor
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#31720 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABSFUCI7NP3PIYOGIO6UFIDSS7EN5ANCNFSM4SJJVMRA>
> .
>
|
|
@perrotta Once this coreHcalPR is merged: this is the plan
The important point in my opinion, this coreHcalPR have reached all the needed ingredients to continue in cmssw w/o passing in cms-patatrack. |
|
Thank you @fwyzard @vkhristenko @mariadalfonso for your investigation and comments For what I can see, what's missing for M0 is just the insertion of a couple of scale factors: isn't it something basically ready to be easily implemented, and that could therefore already enter this PR? Going back at the past presentation at the reco meeting cited by @mariadalfonso: was the issue of the 0 GPU energy finally solved, after that presentation? I agree that all other points listed in #31720 (comment) can be postponed to a forthcoming update PR. Even the two items above can be postponed, if they are really too far from getting a solution now, although if they are ready to be implemented I don't see why not to integrate them here: please let us know. |
yes this has been fixed, see here |
|
@vkhristenko @mariadalfonso please let us know what do you intend to do about the missing scale factor(s) for the method0. That seems really a couple of multiplicative factors that are now missing, and which should be easy to get (if you don't have them already available): please let us know if there are issues I can't imagine that suggest to delay applying the fix to a follow up PR, or if we can implement it already here, instead. |
|
@perrotta
therefore, we would like to postpone and update this after things get merged, especially since it is affecting M0 only and not Mahi. |
|
I'd second Viktor (and Maria) here...
…On Mon, 7 Dec 2020, Viktor Khristenko wrote:
@perrotta
We (me and @mariadalfonso) would prefer that things get merged first.
regarding what's missing exactly and other ref see this patatrack pr
(cms-patatrack#374). In short, we have to add containment correction. And
although its application is a simple mult, to get this double, there is some
logic, which also involves std::map.... which i personally never wanted to
understand and which obviously has to be converted...
* https://github.com/cms-sw/cmssw/blob/master/RecoLocalCalo/HcalRecAlgos/src/
SimpleHBHEPhase1Algo.cc#L172-L178
* https://github.com/cms-sw/cmssw/blob/master/CalibCalorimetry/HcalAlgos/src/
HcalPulseContainmentCorrection.cc#L51
therefore, we would like to postpone and update this after things get
merged, especially since it is affecting M0 only and not Mahi.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, orunsubscribe.[ABGHJWSBI34ENHZ7V4TI7BDSTTMAFA5CNFSM4SJJVMRKYY3PNVWWK3TUL52HS4
DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFQNGLCI.gif]
|
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
I've created #32413 and added a link to cms-patatrack#539 to keep track of the open issues on the HCAL side. |
@mariadalfonso, since you seem so unhappy about the integration via cms-patatrack, I assume all further development will happen only in the master cms-sw repository. What are the plans for keeping the performance of the GPU workflow under control during the development and validation of future PRs, in terms of both physics results and e.g. event processing throughput ? |
|
+1 |
PR description:
Data formats and algorithms for the HCAL local reconstruction running on GPU.
Implements the HBHE unpacking and the production of HCAL uncalibrated rechits, including the MAHI algorithm.
PR validation:
Changes in use in the Patatrack releases.
if this PR is a backport please specify the original PR and why you need to backport that PR:
Includes changes from: