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

create pT2 object in Segment Linking #138

Draft
wants to merge 5 commits into
base: CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel
Choose a base branch
from

Conversation

YonsiG
Copy link

@YonsiG YonsiG commented Dec 4, 2024

This is a draft PR of creating pT2 in Segment Linking

@YonsiG YonsiG marked this pull request as draft December 4, 2024 13:00
Copy link

@slava77 slava77 left a comment

Choose a reason for hiding this comment

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

this is up to RecoTracker/LSTCore/standalone/code/core/AccessHelper.
I'm not sure I'll get to the rest later today

@@ -12,6 +12,7 @@ void lst::Event<Acc3D>::init(bool verbose) {
trackCandidatesInGPU = nullptr;
pixelTripletsInGPU = nullptr;
pixelQuintupletsInGPU = nullptr;
PT2sInGPU = nullptr;
Copy link

Choose a reason for hiding this comment

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

variables and functions should start with lower case, upper case is for type names

unsigned int nInnerSegments;
auto nInnerSegments_src_view = alpaka::createView(devHost, &nInnerSegments, (size_t)1u);

auto dev_view_nSegments = alpaka::createSubView(segmentsBuffers->nSegments_buf, (Idx)1u, (Idx)nLowerModules_);
Copy link

Choose a reason for hiding this comment

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

this is reading nPLs, right?
It would be better to name accordingly
In some other code pixelModuleIndex is used (which I'm guessing is the same value as nLowerModules_
(this applies to pT3/5 code from where this is copied)


auto dev_view_nSegments = alpaka::createSubView(segmentsBuffers->nSegments_buf, (Idx)1u, (Idx)nLowerModules_);

alpaka::memcpy(queue, nInnerSegments_src_view, dev_view_nSegments);
Copy link

Choose a reason for hiding this comment

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

memcpy method is (queue, destination, source) please rename nInnerSegments_src_view to nInnerSegments_host_view to be less confusing
(this applies to pT3/5 code from where this is copied)

PT2sInGPU->setData(*PT2sBuffers);
}

unsigned int nInnerSegments;
Copy link

Choose a reason for hiding this comment

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

why not call this nPLS or nPixelSegments to match the naming more consistently (this applies to pT3/5 code from where this is copied)

Comment on lines +1001 to +1013
for (unsigned int i = 0; i < nInnerSegments; i++) { // loop over # pLS
int8_t pixelType = pixelTypes[i]; // Get pixel type for this pLS
int superbin = superbins[i]; // Get superbin for this pixel
if ((superbin < 0) or (superbin >= (int)size_superbins) or (pixelType > 2) or (pixelType < 0)) {
connectedPixelSize_host[i] = 0;
connectedPixelIndex_host[i] = 0;
continue;
}

// Used pixel type to select correct size-index arrays
if (pixelType == 0) {
connectedPixelSize_host[i] =
pixelMapping_->connectedPixelsSizes[superbin]; // number of connected modules to this pixel
Copy link

Choose a reason for hiding this comment

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

it looks like the same operations on the host side are done for the third time now.
It would be better to carry the connectedPixel* device buffers from the ones filled in the pT5 creation

Comment on lines +279 to +283
if (segmentsInGPU.partOfPT5[outerSegmentIndex])
continue; //don't create pT2s for T2s accounted in pT5s

if (segmentsInGPU.partOfPT3[outerSegmentIndex])
continue; //don't create pT2s for T2s accounted in pT3s
Copy link

Choose a reason for hiding this comment

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

this is probably the origin of the crashes: partOf* variables are in the pLS sub-SoA, not the OT LS.
This should be removed. If segments (OT LS) used somewhere else should be checked for use, new variables should be added to the OT part of the Segments SoA

Copy link
Author

Choose a reason for hiding this comment

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

I added in the pT3 kernels and pT5 kernels, and check if the segment is used in their buildings. I think these lines are filling the partOfPT3 variable.

segmentsInGPU.partOfPT3[i_pLS] = true;

@@ -41,6 +41,7 @@ namespace lst {

constexpr unsigned int n_max_pixel_md_per_modules = 2 * n_max_pixel_segments_per_module;

constexpr unsigned int n_max_pixel_segments = 15000;
Copy link

Choose a reason for hiding this comment

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

can be pt2 already

Suggested change
constexpr unsigned int n_max_pixel_segments = 15000;
constexpr unsigned int n_max_pt2s = 15000;

RecoTracker/LSTCore/src/alpaka/Segment.h Outdated Show resolved Hide resolved
// ===============

//____________________________________________________________________________________________
unsigned int getPixelLSFrompT2(lst::Event<Acc3D>* event, unsigned int PT2) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
unsigned int getPixelLSFrompT2(lst::Event<Acc3D>* event, unsigned int PT2) {
unsigned int getPixelLSFromPT2(lst::Event<Acc3D>* event, unsigned int pT2) {

}

//____________________________________________________________________________________________
unsigned int getLSsFrompT2(lst::Event<Acc3D>* event, unsigned int PT2) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
unsigned int getLSsFrompT2(lst::Event<Acc3D>* event, unsigned int PT2) {
unsigned int getLSFromPT2(lst::Event<Acc3D>* event, unsigned int PT2) {

one LS

@slava77
Copy link

slava77 commented Dec 4, 2024

RecoTracker/LSTCore/standalone/code/core/AccessHelper.
I'm not sure I'll get to the rest later today

I somewhat quickly checked the rest and didn't find problems; but I'm not particularly familiar with the eff/performance plotting to comment on those parts

YonsiG and others added 2 commits December 6, 2024 15:53
Index off

Co-authored-by: Andres Rios Tascon <[email protected]>
fix the segment logical layers

Co-authored-by: Slava Krutelyov <[email protected]>
@@ -872,6 +872,7 @@ namespace lst {
tripletsInGPU.partOfPT5[quintupletsInGPU.tripletIndices[2 * quintupletIndex + 1]] = true;
segmentsInGPU.partOfPT5[i_pLS] = true;
quintupletsInGPU.partOfPT5[quintupletIndex] = true;
segmentsInGPU.partOfPT5[i_pLS] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Was this added by accident?

Copy link
Author

Choose a reason for hiding this comment

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

It is a duplication. Thanks!

@ariostas
Copy link
Member

@YonsiG the rebased branch is CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel_pT2_Yanxi_rebased. I didn't fully address Slava's comments, but I did fix some things. Let me know if you find something I missed or if you have any questions.

@YonsiG
Copy link
Author

YonsiG commented Dec 11, 2024

@YonsiG the rebased branch is CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel_pT2_Yanxi_rebased. I didn't fully address Slava's comments, but I did fix some things. Let me know if you find something I missed or if you have any questions.

Hi Andres, thank you so much for rebasing this work! I will check the rebase branch. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants