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

needs to split with - #1455

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

needs to split with - #1455

wants to merge 1 commit into from

Conversation

crcrpar
Copy link
Collaborator

@crcrpar crcrpar commented Nov 20, 2024

What does this PR do?

Fixes the lack of the split of string representing alias tensor indices.

Signed-off-by: Masaki Kozuki <[email protected]>
Copy link
Collaborator

@kshitij12345 kshitij12345 left a comment

Choose a reason for hiding this comment

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

I think we should add a test with multiple groups of aliased tensors (which I think would fail on current main).

lambda: f"It seems that {vanilla_tensor_args} are {alias_tensor_indices=} share their storage and some of them are modified in-place",
NotImplementedError,
)
for alias_tensor_group in alias_tensor_indices_str.split("-"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to describe the structure of alias_tensor_indices_str - aliases are comma separated, groups are hyphen separated. (copied from _alias_tensor_of_args_kwargs docstring)

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.

2 participants