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

Matrix copy #22

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Matrix copy #22

merged 1 commit into from
Oct 7, 2019

Conversation

sebpuetz
Copy link
Member

@sebpuetz sebpuetz commented Oct 4, 2019 •
edited
Loading

Not quite #17 but still something that might be nice to have. Mirrors the implementation in finalfusion-python, where I stole the copy_storage_to_array fn from.

@sebpuetz sebpuetz requested a review from danieldk October 7, 2019 09:10
danieldk
danieldk requested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Looks good! Maybe the new functions could be moved to storage.rs and be renamed ff_storage_rows and ff_storage_matrix_copy (or ff_storage_copy), following the example of vocab.rs?

@sebpuetz
Copy link
Member Author

sebpuetz commented Oct 7, 2019

We might also want to think about splitting finalfusion.h in a future PR. As of now, there's a lot going on in there.

@sebpuetz sebpuetz requested a review from danieldk October 7, 2019 11:47
danieldk
danieldk approved these changes Oct 7, 2019
View reviewed changes
Copy link
Member

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

One more small comment, looks good otherwise.

tests/matrix_copy.c Outdated Show resolved Hide resolved
@sebpuetz
Expose matrix through copy and add method for row number.
e5514df 
Add ff_storage_copy method that copies the embedding matrix to a
new buffer. ff_storage_rows is also added to allow iterating
over the embeddings.
@sebpuetz
Copy link
Member Author

sebpuetz commented Oct 7, 2019

I'll merge once CI is happy

@sebpuetz sebpuetz merged commit aaf67ef into master Oct 7, 2019
@sebpuetz sebpuetz deleted the matrix-copy branch October 7, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sebpuetz @danieldk