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

Make Tensor generic and add ShapeMap and Rank types #21

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pparke
Copy link
Contributor

@pparke pparke commented Apr 4, 2024

This PR is an attempt to implement the suggested types from #18
The types are the same as the ones in @tensorflow/tfjs-core and should be adapted to what this project requires if necessary.

TODO

  • Use ShapeMap[R] to replace other instances of Array<any>
  • Fix type errors in At.backward
  • Fix type errors in tril
  • Narrow types further if possible

Some of the other support functions may need to be updated in order to resolve the type errors. It would help to properly define types for the other functions that currently return any or Array<any>. In depth knowledge of how those functions is needed and if the logic is changed we should first add unit tests that can be used to check if the output remains the same for all cases.

@eduardoleao052
Copy link
Owner

eduardoleao052 commented Apr 4, 2024

Thank you for the help! In the cases where the any issue still remains, I'll try to fix it, even if it requires changing the function's logic.

@eduardoleao052
Copy link
Owner

Just created a new branch for this. I changed your interface to NDArray. The problem right now is that when I check if an object of type NDArray is an Array, TypeScript does not know that.
Screenshot 2024-04-04 at 17 26 08

@eduardoleao052
Copy link
Owner

@pparke Also, yes we can remove the null from assure array. It should receive only:

  • NDArray(Arrays of rank 1->5)
  • Tensor(Tensor of rank 1->5)
  • Number
    And should return only:
  • NDArray (Arrays of rank 1 if input was number, or Array of input's rank if input was Tensor or NDArray)

Do you think we could pull that off? The main problem currently is what I described. TS does not seem to understand that a NDArray is necessarily an Array...

@eduardoleao052
Copy link
Owner

@pparke also, I don't think I made that clear, but in the current implementation in the types branch I just changed the name of your type from ShapeMap to NDArray.

@eduardoleao052
Copy link
Owner

@pparke However, I think I now see what's happening here. ShapeMap, in tensorflow-js, is used as the type for the Shapes. The type of the data is ArrayMap.

@pparke
Copy link
Contributor Author

pparke commented Apr 9, 2024

@eduardoleao052 yes you are correct, it looks like I mixed up the two. Sorry about that, I only spent a few minutes setting up this draft PR so I must not have looked closely enough at their implementation. It seems like they don't even store the data in the objects they create which makes sense since the processing is done on GPU. When they need to return the data they call an async function to fetch it and then they transform it to the correct shape and return an ArrayMap https://github.com/tensorflow/tfjs/blob/f0f981fe306bf548e300536aca485c0ffdd6619e/tfjs-core/src/tensor.ts#L326

I'm away travelling at the moment so I won't be able to do much this week but it looks like you're getting a good grasp on the data types. Feel free to close this PR if you've already made a lot of progress in another branch, it's only a draft anyways so not a lot of effort went into it.

@eduardoleao052
Copy link
Owner

@pparke
Ok! The draft is being really useful! I'm trying to build upon it, so whenever you want to work on it I'll try to store the latest version of what I'm working on in the types branch.

Thanks again for all the help!

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.

None yet

2 participants