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.

Sign up for GitHub

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

Jump to bottom

Support slicing in MultiEmbeddingTensor #217

Merged
merged 18 commits into from
Nov 18, 2023
Merged

Support slicing in MultiEmbeddingTensor #217

merged 18 commits into from
Nov 18, 2023

Conversation

akihironitta
Copy link
Member

@akihironitta akihironitta commented Nov 15, 2023 •
edited
Loading

Part of #88. Unblocks #194 and #206.

@akihironitta akihironitta self-assigned this Nov 15, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 15, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4e350e3) 92.29% compared to head (44de259) 92.53%.

❗ Current head 44de259 differs from pull request most recent head f0ed686. Consider uploading reports for the commit f0ed686 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   92.29%   92.53%   +0.23%     
==========================================
  Files         109      109              
  Lines        5061     5210     +149     
==========================================
+ Hits         4671     4821     +150     
+ Misses        390      389       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot removed the data label Nov 15, 2023
@github-actions github-actions bot added the data label Nov 17, 2023
@akihironitta akihironitta changed the title [WIP] Support slicing in MultiEmbeddingTensor Support slicing in MultiEmbeddingTensor Nov 17, 2023
@akihironitta akihironitta marked this pull request as ready for review November 17, 2023 06:58
yiweny
yiweny reviewed Nov 17, 2023
View reviewed changes
test/data/test_multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Outdated Show resolved Hide resolved
weihua916
weihua916 reviewed Nov 17, 2023
View reviewed changes
Copy link
Contributor

@weihua916 weihua916 left a comment

Choose a reason for hiding this comment

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

LGTM in general, thanks!

i think we can put some general methods to parent class _MultiTensor to allow more code sharing with MultiNestedTensor.

test/data/test_multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Show resolved Hide resolved
akihironitta and others added 4 commits November 18, 2023 11:10
@akihironitta @weihua916
@zechengz @yiweny
Address comments
4275dd4 
Co-authored-by: Weihua Hu <weihua916@gmail.com>
Co-authored-by: Zecheng Zhang <zecheng@kumo.ai>
Co-authored-by: Yiwen Yuan <yyuanlisette@gmail.com>
@akihironitta
add one more test case
f66ae81
@akihironitta @weihua916
Update validate
c5423a6 
Co-authored-by: Weihua Hu <weihua916@gmail.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
44de259 
for more information, see https://pre-commit.ci
weihua916
weihua916 approved these changes Nov 18, 2023
View reviewed changes
Copy link
Contributor

@weihua916 weihua916 left a comment

Choose a reason for hiding this comment

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

LGTM after avoiding for-loop via batched_arange (see here). Thank you.

torch_frame/data/multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Outdated Show resolved Hide resolved
torch_frame/data/multi_embedding_tensor.py Show resolved Hide resolved
@akihironitta akihironitta enabled auto-merge (squash) November 18, 2023 19:10
@akihironitta akihironitta merged commit 2d49bd7 into master Nov 18, 2023
10 checks passed
@akihironitta akihironitta deleted the aki/feat-met-slice branch November 18, 2023 19:11
@akihironitta akihironitta mentioned this pull request Nov 21, 2023
Closed
@akihironitta akihironitta mentioned this pull request Dec 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihua916 weihua916 weihua916 approved these changes

@rusty1s rusty1s Awaiting requested review from rusty1s

@yiweny yiweny Awaiting requested review from yiweny

@zechengz zechengz Awaiting requested review from zechengz

Assignees

@akihironitta akihironitta

Labels
0 - Priority P0 data feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akihironitta @weihua916 @zechengz @yiweny