Fix second argument of reorder API to be more clear

[9prady9]

Fixes #202

@srenevey How about the updated API ? Also, do take a look at how new API handles it's second argument.

[srenevey]

I think it's good. Two comments though.

1. In the method's comment, it is mentioned: "The default order of axes in ArrayFire is axis with smallest distance between adjacent elements...". Do you mean smallest distance between memory blocks where the array is stored ? If so it might be worth mentioning it explicitly so as to not confuse anyone with "mathematical distance" such as norm.

2. For the second argument, what is the purpose of using Option<Vec> instead of Vec ? In what situation would someone call reorder(&input, None) ?

Thanks for the prompt fix.

[9prady9]

I will fix the point 1. Regarding 2, I am still contemplating on what could be right way to pass these new axes. If possible, I want to make sure I don't need to change the API if max number of dimensions change in ArrayFire. I had a simple u64 argument earlier which was later removed. I will address this later today.

…

On Mon 23 Sep, 2019, 18:04 srenevey, ***@***.***> wrote: ***@***.**** commented on this pull request. I think it's good. Two comments though. 1. In the method's comment, it is mentioned: "The default order of axes in ArrayFire is axis with smallest distance between adjacent elements...". Do you mean smallest distance between memory blocks where the array is stored ? If so it might be worth mentioning it explicitly so as to not confuse anyone with "mathematical distance" such as norm. 2. For the second argument, what is the purpose of using Option<Vec> instead of Vec ? In what situation would someone call reorder(&input, None) ? Thanks for the prompt fix. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#203?email_source=notifications&email_token=AAY6OOXLMO4OX46FXV7G3PLQLCZVFA5CNFSM4IZGUXJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFR2BSA#pullrequestreview-291741896>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY6OOQVHWKSBYANMJDK2WLQLCZVFANCNFSM4IZGUXJA> .

[9prady9]

@srenevey I think it is more clear now. please have a look.

Changing only one axis doesn't make sense, so I am debating if I take vec4 altogether or what I have now. At least two axes are to be swapped, hence we have two mandatory u64 arguments and then an optional vec of u64's. This way, even if we change max dims support from ArrayFire, we can avoid any change in rust wrapper API.

[srenevey]

Thanks for working on this. If you'd rather have the the signature that you've proposed I'll gladly approve the changes.