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

Issue #925: Added noncopy constructor for Tensor and TensorT #369

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

ThePseudo
Copy link
Contributor

Added methods reserve() and constructor for Tensor and TensorT

@axsaucedo
Copy link
Member

Looks good - only thing would be to add a test, and we can then merge. You can add this on the Tensor test file.

@axsaucedo
Copy link
Member

Just following up, do you think you would be able to have a look with a short test @ThePseudo ?

@ThePseudo
Copy link
Contributor Author

Sorry for the delay, at some point stuff happened and I lost track of a few things (one being this). I created a short test, which could be expanded upon, but is basically "inspired" by the other test already present. I hope this works!

@axsaucedo
Copy link
Member

No worries @ThePseudo, thank you for following up - running test suite

@axsaucedo
Copy link
Member

@ThePseudo it seems tests failing, did you run locally?

@ThePseudo
Copy link
Contributor Author

The terrible thing is that I am not really an expert in the Google test suite... but I found that the tests were failing for a good reason, apparently, and it is building. Which is strange, since "it worked on my machine", but let's try this out

Added methods reserve() and constructor for Tensor and TensorT

Signed-off-by: Andrea Calabrese <[email protected]>
Tensor now has a binding on the manager, allowing to be created from the
manager itself rather than passing arguments to the Tensor class
(similarly to the copy-tensor)

Signed-off-by: Andrea Calabrese <[email protected]>
Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

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

Thank you @ThePseudo - remaining commment on a small docstring change required

Reserved tensor now has a proper test, allocating 3 uint32_t.

Signed-off-by: Andrea Calabrese <[email protected]>
@ThePseudo
Copy link
Contributor Author

Thank you @ThePseudo - remaining commment on a small docstring change required

It should be done now, but tell me if the format is not correct (I took inspiration from previous docstrings)

@axsaucedo
Copy link
Member

Added extra suggestion as it needs to follow the same format as rest of docstrings

@axsaucedo
Copy link
Member

Thank you @ThePseudo - signed commit required, and we should be able to wrap it up :)

As for the title. Suggestion provided by axsaucedo

Co-authored-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Andrea Calabrese <[email protected]>
@ThePseudo
Copy link
Contributor Author

Thank you @ThePseudo - signed commit required, and we should be able to wrap it up :)

Forgot it, now added! :D

@axsaucedo
Copy link
Member

Thanks!

@axsaucedo axsaucedo merged commit 1748b82 into KomputeProject:master Aug 20, 2024
8 checks passed
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.

2 participants