Skip to content

Add support for ellipsis (...) indexing in Helion #437

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

Closed
wants to merge 1 commit into from

Conversation

yf225
Copy link
Contributor

@yf225 yf225 commented Aug 6, 2025

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 6, 2025
yf225 added a commit that referenced this pull request Aug 6, 2025
stack-info: PR: #437, branch: yf225/stack/53
Comment on lines +453 to +458
def _calculate_ellipsis_dims(
index: list[object], current_index: int, total_dims: int
) -> int:
"""Calculate how many dimensions an ellipsis should expand to."""
remaining_indices = len(index) - current_index - 1
return total_dims - current_index - remaining_indices
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right when combined with None (which will increment i in the caller but not consume an input dimension.

Comment on lines +230 to +232
if k is Ellipsis:
# Ellipsis is not supported in tensor descriptor mode
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this hard to support? Maybe we we pre-expand the indexing string the existing code could work unmodified.

stack-info: PR: #437, branch: yf225/stack/53
@yf225
Copy link
Contributor Author

yf225 commented Aug 10, 2025

After thinking more about it: support is probably not consistent with the general mental model of “must specify all dims” in Helion tensor indexing. So will skip this feature for now.

@yf225 yf225 closed this Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants