Skip to content

feat: allow variable tensor dimension#1007

Merged
samsja merged 17 commits intofeat-rewrite-v2from
feat-871-tensor-vardim
Jan 16, 2023
Merged

feat: allow variable tensor dimension#1007
samsja merged 17 commits intofeat-rewrite-v2from
feat-871-tensor-vardim

Conversation

@Jackmin801
Copy link
Copy Markdown
Contributor

@Jackmin801 Jackmin801 commented Jan 11, 2023 •
edited
Loading

Goals:

@Jackmin801 Jackmin801 force-pushed the feat-871-tensor-vardim branch from abd14a0 to 5468d0b Compare January 11, 2023 10:46
@github-actions github-actions bot added size/m and removed size/s labels Jan 11, 2023
@Jackmin801
Copy link
Copy Markdown
Contributor Author

Jackmin801 commented Jan 11, 2023

Do we want to add an example usage in the docstrings?

@Jackmin801 Jackmin801 linked an issue Jan 11, 2023 that may be closed by this pull request
Add TorchTyping like features #871
Closed
JohannesMessner
JohannesMessner requested changes Jan 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@JohannesMessner JohannesMessner left a comment

Choose a reason for hiding this comment

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

Can this logic be move into the AbstractTensor class so that it "just works" when adding a new tensor type? Also I'm guessing variables ('x') are not shared between tensors (or the same Document), right?

@JohannesMessner
Copy link
Copy Markdown
Member

JohannesMessner commented Jan 11, 2023

Do we want to add an example usage in the docstrings?

Yes please!

@Jackmin801
Copy link
Copy Markdown
Contributor Author

Jackmin801 commented Jan 11, 2023 •
edited
Loading

Can this logic be move into the AbstractTensor class so that it "just works" when adding a new tensor type? Also I'm guessing variables ('x') are not shared between tensors (or the same Document), right?

First question, unfortunately not. Numpy and torch have slightly different api, particularly in this case, in the view vs reshape. And yep, the 'x' isn't shared.

samsja
samsja reviewed Jan 11, 2023
View reviewed changes
samsja
samsja reviewed Jan 11, 2023
View reviewed changes
samsja
samsja reviewed Jan 11, 2023
View reviewed changes
samsja
samsja reviewed Jan 11, 2023
View reviewed changes
samsja
samsja reviewed Jan 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@samsja samsja left a comment

Choose a reason for hiding this comment

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

would be nice to find a way to factorize the code between torch and numpy

@samsja
Copy link
Copy Markdown
Member

samsja commented Jan 11, 2023 •
edited
Loading

First question, unfortunately not. Numpy and torch have slightly different api, particularly in this case, in the view vs reshape. And yep, the 'x' isn't shared.

cannot we abstract the reshaping part, somewhere in the computational backend ?

@JohannesMessner
Copy link
Copy Markdown
Member

JohannesMessner commented Jan 11, 2023

First question, unfortunately not. Numpy and torch have slightly different api, particularly in this case, in the view vs reshape. And yep, the 'x' isn't shared.

cannot we abstract the reshaping part, somewhere in the computational backend ?

I suggest the same

@Jackmin801
test: add torch tensor tests
b5f98f4 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
feat: implement for torch tensor
f7be0c0 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
test: add numpy array tests
429b6bf 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
feat: implement for numpy array
4fdf3d5 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
test: add uncovered edge case
c70376e 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
fix: fix uncovered edge case
f3635e5 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
docs: showcase variable dimension usage
26970e6 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
fix: small typo
8b7dda3 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
docs: add variable dim example to docstrings
c745629 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801
refactor: single quote string normalization
6f89780 
Signed-off-by: Jackmin801 <[email protected]>
@Jackmin801 Jackmin801 force-pushed the feat-871-tensor-vardim branch from 01119e5 to 6f89780 Compare January 16, 2023 08:03
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
@Jackmin801 Jackmin801 force-pushed the feat-871-tensor-vardim branch from b07cc2f to 5c87da1 Compare January 16, 2023 12:35
@Jackmin801 Jackmin801 marked this pull request as draft January 16, 2023 12:40
@Jackmin801 Jackmin801 marked this pull request as ready for review January 16, 2023 13:16
samsja
samsja reviewed Jan 16, 2023
View reviewed changes
docarray/computation/torch_backend.py
"""
...

@overload
Copy link
Copy Markdown
Member

@samsja samsja Jan 16, 2023

Choose a reason for hiding this comment

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

Would be nice to automatically generate the overload. But lets not do it in this PR unless you found a brillant solution haha

Copy link
Copy Markdown
Contributor Author

@Jackmin801 Jackmin801 Jan 16, 2023

Choose a reason for hiding this comment

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

I dont think it works :( python/mypy#11488

Copy link
Copy Markdown
Member

@samsja samsja Jan 16, 2023

Choose a reason for hiding this comment

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

but we could generate the code though

Copy link
Copy Markdown
Member

@samsja samsja Jan 16, 2023

Choose a reason for hiding this comment

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

but yeah lets see later

Copy link
Copy Markdown
Member

@samsja samsja Jan 16, 2023

Choose a reason for hiding this comment

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

nah not code generation via our friend the bot.

But we could have a helper function that generate 3 methods with the correct overload

Copy link
Copy Markdown
Member

@JohannesMessner JohannesMessner Jan 16, 2023

Choose a reason for hiding this comment

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

could it be done via decorator somehow?

@Jackmin801 Jackmin801 force-pushed the feat-871-tensor-vardim branch from 7ab4d2e to ecdf48a Compare January 16, 2023 14:24
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 16, 2023

📝 Docs are deployed on https://ft-feat-871-tensor-vardim--jina-docs.netlify.app 🎉

@samsja samsja merged commit 116bca9 into feat-rewrite-v2 Jan 16, 2023
@samsja samsja deleted the feat-871-tensor-vardim branch January 16, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohannesMessner JohannesMessner JohannesMessner approved these changes

@samsja samsja samsja approved these changes

Assignees

No one assigned

Labels

area/core area/testing area/typing size/m

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add TorchTyping like features

3 participants

@Jackmin801 @JohannesMessner @samsja