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

wishlist #69

Closed
ZacDiggum opened this issue Mar 24, 2016 · 15 comments
Closed

wishlist #69

ZacDiggum opened this issue Mar 24, 2016 · 15 comments

Comments

@ZacDiggum
Copy link
Contributor

  • cast numpy scalars to af scalars, so that things like np.sin(scalar)+af_array=np_array don't happen. It's confusing and people are bound to use math. or cmath. functions. Pyopencl does the casting silently. funny: af_array+np.sin(scalar)=af_array
  • change array layout to row-major as in numpy and C/C++ so that arrays copied from the host and arrays created on the device match (or is there a reason for column-major arrays?))
  • make the api a little more like numpy (I know, afnumpy is there) e.g. implement more array methods (.abs(), .min(), .max(), .sum(), .imag, .real, .conj(), .transpose() and so on) and rename the array attributes (.elements() to .size, .dims() to .shape, af.moddims() to .reshape()...)
@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@ZacDiggum

  • (1) may be a bug. np.sin(scalar) + af_array is perhaps calling the __add__ on np.ndarray but not on af.Array. I will see if I can figure this out.
  • (2) There is an issue for this: Add support for C contiguous arrays #56. The problem is arrayfire library expects the data in the fortran format. For this reason it is not a trivial thing to implement but I am looking into it.
  • (3) Adding new methods is fine, but I don't like the idea of renaming attributes. The goal of the wrapper is to be consistent with other arrayfire language functions when it comes to naming things. You could always have a child class that inherits from af.Array in your code that does the renaming.

@FilipeMaia any thoughts ?

@FilipeMaia
Copy link
Contributor

  • (1) np.sin(scalar) + af_array definitely calls __add__ on numpy. Probably then numpy tries to convert the second argument to a numpy array by calling __array__ on it. The exact same issue exists in afnumpy. I don't know of a way around this while still ensuring that np.array(af_array) works.
  • (3) I would actually avoid adding multiple names for the same function. It makes the namespace more busy and confusing.

@FilipeMaia
Copy link
Contributor

BTW about (1) the issue only happens with numpy. functions, not the ones in math. or cmath. which return normal python scalars.

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@FilipeMaia Any idea why numpy's __add__ takes precedence over arrayfires __radd__ ?

@FilipeMaia
Copy link
Contributor

@pavanky Why would your class have precedence over numpy's one? The numpy argument is first so its __add__ is always the first to be called. Only in case that it fails is __radd__ called on the other argument.

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@FilipeMaia yeah I just found that out myself. May be a simple solution would be to have the user set a flag which chooses between __array__ or not ?

@FilipeMaia
Copy link
Contributor

@pavanky That could be an option. Another possibility might be to raise a warning if __array__ is being called by someone other than numpy.array, although this might require some ugly stack introspection.

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@FilipeMaia This will still not solve the problem. I am guessing np.ndarray.__add__ calls __array__ which would result in no warning at all.

@FilipeMaia
Copy link
Contributor

You mean __add__ calls numpy.array. Yes you're probably right, so we would need to be clever about it...

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@FilipeMaia I am leaning towards not supporting __array__ by default and only enabling it when a user manually asks for it. We could have a method to_ndarray which users can call conversion to numpy.

@FilipeMaia
Copy link
Contributor

Sure, that's cleaner and also a reasonable solution.

@FilipeMaia
Copy link
Contributor

@pavanky I found a better solution! Setting the __array_priority__ attribute in Array. I tested this on afnumpy and it seems to work.
Check http://stackoverflow.com/questions/35866738/avoid-numpy-distributing-an-operation-for-overloaded-operator

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@FilipeMaia can you send in a patch ? You understand the situation better than I do here.

@FilipeMaia
Copy link
Contributor

@pavanky #71 should solve the first issue.

@pavanky
Copy link
Member

pavanky commented Mar 24, 2016

@ZacDiggum since (1) is now fixed and (2) has a different issue associated with it, can you close the issue and open a new one for adding new methods (or atleast rename this issue to reflect the unresolved ones).

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

No branches or pull requests

3 participants