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

Implement Tensor struct #2

Merged
merged 24 commits into from
Jun 30, 2024
Merged

Implement Tensor struct #2

merged 24 commits into from
Jun 30, 2024

Conversation

siliconlad
Copy link
Member

No description provided.

@siliconlad siliconlad self-assigned this Jun 20, 2024
@siliconlad siliconlad linked an issue Jun 20, 2024 that may be closed by this pull request
@siliconlad
Copy link
Member Author

@CameronMatthew can you have a look and see if this is heading in the right direction.

Very rough around the edges (can you flatten an n-dimensional tensor in your head?), but that can be patched up with refinements to the API and maybe some additional macros.

Also code is a bit of a mess (wth is iters.rs) but gotta start somewhere

Copy link
Contributor

@CameronMatthew CameronMatthew left a comment

Choose a reason for hiding this comment

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

Definitely along the right lines! This is very nice. One or two places you can make your life easier. Would also like to talk about reusing some of this for Static tensors

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link

@Dougal-s Dougal-s left a comment

Choose a reason for hiding this comment

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

I'm very impressed by how quickly you managed to put this together!

src/iter.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/shape.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@siliconlad
Copy link
Member Author

siliconlad commented Jun 26, 2024

I have restructured the code to have some of the features that we want. NamelyDynamicStorage which abstracts away the memory mapping of the tensor. I have also tweaked some of the interfaces for the methods as mentioned above (e.g. returning Result in various places).

I'm going to press ahead with some further restructurings:

  • Create dedicated Vector and Matrix types which support some extra methods
  • Create a distinction between int and float Tensors (to hopefully support pow)

I'm ignoring the specific implementations of the methods for now because they work (even if they are over-complicated). I will revisit them at the end and tidy them up as well as implement any of the other missing functionality.

@siliconlad
Copy link
Member Author

siliconlad commented Jun 28, 2024

Would be good to have another review of the code written so far. I've implemented most of the methods outlined in #1 so far (missing more of the advanced features). Before I add any more, would like to get some feedback on the structure of everything. The way I've done it seems to work, but there's a fair bit of duplication. Perhaps there's a better way.

Largely ignore the code within the methods for now. A lot of it needs to be looked at and refactored (unless you want to suggest a way to refactor it). They do get the job done though so 🤷 (I've never had more tests than I have for this repository I don't think ahaha).

@CameronMatthew @Dougal-s

@CameronMatthew
Copy link
Contributor

CameronMatthew commented Jun 29, 2024

I had a quick look and seems fine. I think we should just merge this asap now.

For the following reasons...

  • Something "imperfect" is much better than nothing.
  • This PR is getting too large to review properly.
  • While this remains on a dev branch, the work you do on it is so broadly scoped that it is impossible for others to contribute without risking massive merge conflicts.
  • Merging will allow us to create small, self-contained issues that we can all pick up, work on and review easily.

TL;DR Ship it 🚢!

@siliconlad
Copy link
Member Author

Sounds good to me.

Copy link
Contributor

@CameronMatthew CameronMatthew left a comment

Choose a reason for hiding this comment

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

LGTM!

src/matrix.rs Show resolved Hide resolved
@siliconlad siliconlad merged commit 87c2d59 into main Jun 30, 2024
5 checks passed
@siliconlad siliconlad deleted the 1-implement-tensor branch June 30, 2024 18:47
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.

Implement Tensor
3 participants