-
Notifications
You must be signed in to change notification settings - Fork 61
DataContainer agnostic on backend
#2132
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: MikeSullivan7 <[email protected]>
Co-authored-by: MikeSullivan7 <[email protected]> Co-authored-by: Gemma Fardell <[email protected]>
Co-authored-by: Gemma Fardell <[email protected]>
Co-authored-by: Hannah Robarts <[email protected]> Co-authored-by: MikeSullivan7 <[email protected]>
slightly relax tolerance of tests
…i_compat and fix numpy specific unit test
|
I had to fix 2 unittest errors which depend on code I do not think I changed. It relates to geometry geometry.dtype = complex
data = geometry.allocate(dtype=np.int64)
self.assertEqual(data.dtype, np.int64)The error was due to the fact that the newly allocated |
| array = numpy.empty(geometry.shape, dtype) | ||
|
|
||
| xp = array_api_compat.numpy | ||
| dtype = kwargs.get('dtype', xp.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you intend to change the default behaviour from dtype = geometry.dtype to xp.float32?
|
|
||
| # defaults to a np array | ||
| xp = np | ||
| dtype = kwargs.get('dtype', xp.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I enjoy array api efforts! I believe @lucascolley likes hearing about array api efforts. Scipy is a great place to look for work done since they have been doing quite a bit of array api work. I added some small comments.
Some general pointers:
-
array-api-strict is a numpy wrapper that enforces all array api standard requirements, so it's good to test your implementation against as a first pass.
-
array-api-extra has a few extra tools for array api stuff that can be useful.
-
The general principle that scipy is taking, and that I think is a very solid approach is
array_in = array_outto the extent that is possible; the idea being that whatever arrays the user inputs to any library function should be the output of that function.
| axis = list(axis) | ||
| axis.sort(reverse=True) | ||
| ax = axis.pop(0) | ||
| if len(axis) == 1: | ||
| axis = axis[0] | ||
| return squeeze(xp.squeeze(array, axis=ax), axis=axis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason to not do
axis = tuple(axis)
return xp.squeeze(array, axis=axis)| xp = array_namespace(array) | ||
| # find and remove singleton dimensions | ||
| if axis is None: | ||
| s = xp.nonzero(xp.asarray(array.shape) == 1)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably better to do this in pure python, particularly since this function might not be in certain lazy libraries due to being data dependent:
s = tuple(i for i, ax in enumerate(array.shape) if ax==1)| xp = array_namespace(a.as_array()) | ||
| if array_namespace(b.as_array()) != xp: | ||
| raise TypeError('Can only compare arrays ' \ | ||
| 'with same namespace. Got {} and {}'.format(array_namespace(a), array_namespace(b))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use
xp = array_namespace(a, b)|
Thanks for the ping @purepani ! I highly recommend reading scipy/scipy#18286 for a feel of the direction we are heading in with SciPy. Please feel free to ping me with any questions. |
| from array_api_compat import array_namespace | ||
| import sys | ||
|
|
||
| def expand_dims(array, axis): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import cupy as cp | ||
| out = ImageData(cp.empty(x.shape, dtype=cp.float32), geometry=self.gradient_operator.domain_geometry().copy(), dtype=cp.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this?
Description
Removes all the checks on the array wrapped by the
DataContainerExample Usage
❤️ Thanks for your contribution!