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

[DEL] Remove .from_inner2 #38

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

[DEL] Remove .from_inner2 #38

wants to merge 1 commit into from

Conversation

f-dangel
Copy link
Owner

Resolves #37.

@f-dangel f-dangel requested a review from runame October 26, 2023 21:46
Copy link
Collaborator

@runame runame left a comment

Choose a reason for hiding this comment

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

LGTM. One question: are there not some cases where this can be more efficient, i.e. when batch_size * seq_len >> feature_dim?

@f-dangel
Copy link
Owner Author

f-dangel commented Oct 27, 2023

Yes, you are right that there are different pros and cons. I tried to sum them up in the following note:

from_inner.pdf

Actually, having done the analysis I think we need to discuss whether we should aim for simplicity by supporting only from_inner, or should dynamically switch between from_inner versus from_inner2 depending on batch_size, sequence_length, feature_dim. In the scenario you mention above, using the from_inner2 approach might save a lot of memory.

For now, I will not merge and label this for 'needs discussion'.

@f-dangel
Copy link
Owner Author

@yorkerlin please add this to the agenda for our next in-person discussion

@f-dangel f-dangel marked this pull request as draft November 13, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate .from_inner2 from StructuredMatrix interface
2 participants