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.
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
feat: Add CUDA connected components & track building #4015
base: main
Are you sure you want to change the base?
feat: Add CUDA connected components & track building #4015
Changes from 5 commits
3b39f73
c789c15
63e23a5
620279f
9725eb2
cee43f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
💡 Codebase verification
Hmmmm, wisdom in error handling I sense, but better approach we must take.
Follow the ways of TorchMetricLearning and TorchEdgeClassifier, you should. Device ID in configuration and ACTS_WARNING they use, yes.
deviceID
to Config struct, you must🔗 Analysis chain
Handle CUDA device initialization gracefully, young padawan must.
In constructor, catch CUDA errors you should, when device initialization fails it might. Use CUDA_CHECK macro from CudaUtils.cuh, wisdom this would be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
Length of output: 5533
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.
I don't know what the type of
TEdge
is, but if it is big you might want to implement this using moves.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.
Actually, since this is a
__device__
function, is there any realistic scenario where a object used on device that has a move constructor? I naivly would not expect this, but I don't have a lot of experience here...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.
This vector can be const.
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.
I think there is some dumb reason that
ONNX
runtime accepts only mutable pointers or so... Probably in that case it would be better to just copy the data, but I wouldn't touch it in this PRThere 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.
Ah but this is graph building... so indeed it could be const
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.
Investigate the TODO comment, you must.
An issue not detected in unit tests, the increment of 'numberLabels' suggests. Rather than adjusting manually, find and fix the root cause, we should.
Assist you, can I. Help investigate this issue or open a new GitHub issue, would you like?