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

The direction of merging axes in transformation shouldn't matter #297

Open
jjsjann123 opened this issue Aug 15, 2020 · 2 comments
Open

The direction of merging axes in transformation shouldn't matter #297

jjsjann123 opened this issue Aug 15, 2020 · 2 comments
Labels
backlog low priority

Comments

@jjsjann123
Copy link
Collaborator

🚀 Feature

currently when we call TensorView::merge(int axis), it accumulates the non-contiguous-ness and stops dimension collapsing once it hits the first non-contig axis.

This is due to the limitation of our implementation under the hood. To better address this trade-off, we prioritize dimension collapsing for faster dimension in PR #294

e.g. for a tensor with a non-contig in the middle like

x = torch.randn(8, 8, 8, 8)
x = x[:, ::2, :, :]

If we merge from left to right as in

for (x_tensor_view->nDims() > 1) {
  x_tensor_view->merge(0);
}

We'll have collapsing from dim0 to dim1, but nothing on the right hand side;

vs merging from right to left

for (x_tensor_view->nDims() > 1) {
  x_tensor_view->merge(-2);
}

We'll have collapsing from dim2 to dim3 (and dim3 to squeeze out in the kernel, because of stride 1) instead.

In the long term, we should be able to properly recognize contiguous axes and collapsing both sides. Starting this thread to track the issue.

@csarofeen
Copy link
Owner

Agreed this should be fixed, but fixing it is unfortunately tricky. We'd likely need a sense of merge reordering, will have to think about this more to figure out if there's an easier/more natural change to address this.

@csarofeen
Copy link
Owner

This remains to behave with limitations that could be lifted. Cross referencing: #262

@jjsjann123 jjsjann123 added the backlog low priority label Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog low priority
Projects
None yet
Development

No branches or pull requests

2 participants