Patatrack integration - ECAL local reconstruction (7/N) #31719
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For all questions, please address @amassiro @vkhristenko . |
|
The code-checks are being triggered in jenkins. |
|
enable gpu |
|
-code-checks ERROR: Build errors found during clang-tidy run. |
fwyzard
added a commit
to fwyzard/cms-bot
that referenced
this pull request
Oct 10, 2020
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)
fwyzard
added a commit
to fwyzard/cms-bot
that referenced
this pull request
Oct 10, 2020
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)
|
@fwyzard |
|
please test with #31704 |
|
So... it looks like it's not possible to run the code checks together with an other PR ? Suggestions on how to proceed ? |
|
PRs should be testable. An option would be to improve the testing framework, but I'm not sure if it's a big additional complexity there to reliably test multiple PRs together. Any other suggestion than progressing with #31704, so that this PR here can be tested automatically? |
See #27983 for a single fully testable PR. |
jpata
reviewed
Oct 15, 2020
CUDADataFormats/EcalRecHitSoA/interface/EcalUncalibratedRecHit.h
Outdated
Show resolved
Hide resolved
| static constexpr int kTowersInPhi = 4; // see EBDetId | ||
| static constexpr int kCrystalsInPhi = 20; // see EBDetId | ||
|
|
||
| static constexpr uint8_t MAX_DCCID = 54; //To be updated with correct and final number |
There was a problem hiding this comment.
In general, for all such values and classes that are reimplemented & copied from the CPU version, I would like to ask to put a reference to the original CPU code in the GPU code comments, both to facilitate review right now and to allow future visitors to compare the two versions.
There was a problem hiding this comment.
This can be a few lines of comments in the top of the file, referencing the original CPU implementations.
There was a problem hiding this comment.
I would actually go further and request that all common values and types be shared between the two implementations.
However, this will likely require changes to the original CPU code to expose such values and types.
| } | ||
| } | ||
|
|
||
| __forceinline__ __device__ void print_first3bits(uint64_t const* buffer, uint32_t size) { |
There was a problem hiding this comment.
as above, put them in a central place
There was a problem hiding this comment.
printing 3 specific bits out of an 8-byte word is defnitely not something of central interest
| auto* pChannelsCounter = isBarrel ? &pChannelsCounterEBEE[0] : &pChannelsCounterEBEE[1]; | ||
|
|
||
| // FIXME: debugging | ||
| //printf("ifed = %u fed = %d offset = %u size = %u\n", ifed, fed, offset, size); |
There was a problem hiding this comment.
remove all commented-out debugging lines here and elsewhere
| sampleValues[7] = wdata2 & 0x3fff; | ||
| sampleValues[8] = (wdata2 >> 16) & 0x3fff; | ||
| sampleValues[9] = (wdata2 >> 32) & 0x3fff; | ||
| //printf("stripid = %u xtalid = %u\n", stripid, xtalid); |
|
The code-checks are being triggered in jenkins. |
|
Include guards and relative comments should be fixed by the latest commit. |
|
-1 Failed Tests: RelVals-INPUT RelVals-INPUTComparison SummarySummary:
|
|
The failures are due to |
|
looks like eos glitch otherwise file is available |
|
Let's rerun the test after some time (e.g. tonight?) to get the final greenlight. |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a29982/12313/summary.html Comparison SummarySummary:
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a29982/12313/summary.html Comparison SummarySummary:
|
|
+reconstruction
|
|
+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 be automatically merged. |
cmsbuild
added
fully-signed
operations-approved
orp-approved
and removed
operations-pending
orp-pending
pending-signatures
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description:
Data formats and algorithms for the ECAL local reconstruction running on GPU.
Implements the ECAL unpacking and the production of ECAL uncalibrated rechits, including the multifit 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: