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

stride() is inconsistent with dimension(), extent(), etc. #1214

Closed
ibaned opened this issue Nov 3, 2017 · 8 comments
Closed

stride() is inconsistent with dimension(), extent(), etc. #1214

ibaned opened this issue Nov 3, 2017 · 8 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@ibaned
Copy link
Contributor

ibaned commented Nov 3, 2017

Why does View::stride() take an array and fill in all the values while View::dimension() and View::extent() each take an index and return one value? This seems really inconsistent and less friendly to use. Can we add a stride() method that just takes an index and returns one value?

@hcedwar @crtrott

@ibaned ibaned added the Enhancement Improve existing capability; will potentially require voting label Nov 3, 2017
@ibaned ibaned added this to the 2017 December milestone Nov 3, 2017
@dholladay00
Copy link

I had this conversation recently and I recall it dealing with the cost of stride per dimension, but I don’t remember all of the details.

@ibaned
Copy link
Contributor Author

ibaned commented Nov 4, 2017

hmm I guess fundamentally stride() is multiplying out all the dimensions, so doing it all at once is more efficient that calling them all one at a time, so I guess I understand the motivation a bit more. still, in my case I only need one, so for that special case its less efficient.

@dholladay00
Copy link

I’m not sure it’s a special case. I think it’s fairly common when interfacing views with linear algebra libraries. My comment wasn’t trying to discourage the feature, I was just reiterating the explanation I received when I asked this question.

@mhoemmen
Copy link
Contributor

mhoemmen commented Nov 5, 2017

stride was really annoying in the past, in that it required an array of 8 entries as output, regardless of the View's rank. I'm hoping that is no longer the case.

@ibaned
Copy link
Contributor Author

ibaned commented Nov 5, 2017

@mhoemmen as far as I know thats still the case, thats why I opened this issue.

Edit: lies.

@dholladay00
Copy link

I was able to do the following without issues:

View<double**> A("A", M, N); // 2D view
int A_stride[A.Rank]; // length 2 array
A.stride(A_stride); // can pass length 2 array int stride call

@ibaned
Copy link
Contributor Author

ibaned commented Nov 6, 2017

Oh yes, I misspoke there. I think the requirement has shifted to only being sized by the rank, so yes that particular issue has been fixed. I'm only referring to the need to pass an array at all.

@dholladay00
Copy link

Yep, I would like to be able to do A.stride(1), etc. to be able to get a specific stride as I only need 1 of the strides for linear algebra calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

6 participants