-
Notifications
You must be signed in to change notification settings - Fork 106
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
ENH: add cupy tensor space #1231
base: master
Are you sure you want to change the base?
Conversation
Looks like a trivial review, I'll try it tomorrow. Does this work with astra data containers? |
Sure, go ahead. It's work in progress though. |
I haven't tried ASTRA interoperability yet. |
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.
Frankly only some minor stuff left, looks freaking magic to me!
odl/space/cupy_tensors.py
Outdated
else: | ||
from pkg_resources import parse_version | ||
if parse_version(cupy.__version__) < parse_version('2.0.0rc1'): | ||
raise ImportError('cupy <2.0.0rc1 not supported') |
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.
This should be a warning, we don't want to render ODL unusable due to a wrong version.
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.
The thing is that version 2.0.0rc1 adds support for complex arrays, I really don't want anything below. Actually we should go for 2.0.0, the latest one on PyPI. It wasn't out at the time of writing this.
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.
Okay, complex stuff is available in 2.0.0, I'll emit a warning for earlier versions.
odl/space/cupy_tensors.py
Outdated
# --- Space method implementations --- # | ||
|
||
|
||
lico = cupy.ElementwiseKernel(in_params='T a, T x, T b, T y', |
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.
Are these lazily evaluated? If not, we should be careful about the startup cost.
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.
They should be, since T
is a type template, so the only alternative would be to compile for all types, which I cannot imagine. The good thing is that the kernels are cached on disk, so after the first call things run way faster.
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 just checked. When I removed pkg_resources
(bloody slow, we should avoid it whenever possible, at least during import odl
), the import time of cupy_tensors.py
is practically identical to the time of import cupy
(~40 ms).
odl/space/cupy_tensors.py
Outdated
elif np.dtype(dtype) == 'float64': | ||
prefix = 'd' | ||
else: | ||
raise ValueError('dtype {!r} not supported by cuBLAS'.format(dtype)) |
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.
Not even complex values?
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.
cuBLAS itself supports it, maybe the bindings are not implemented yet in cupy. I'll check.
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.
Supported in cupy 2.0.0
odl/space/cupy_tensors.py
Outdated
|
||
class CupyTensorSpace(TensorSpace): | ||
|
||
"""Tensor space implemented with GPU arrays. |
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.
This should mention what backend is used.
self.__weighting = CupyTensorSpaceCustomNorm(norm) | ||
elif inner is not None: | ||
self.__weighting = CupyTensorSpaceCustomInner(inner) | ||
else: # all None -> no weighing |
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.
What happened to our no weighting vs const weighting debate? I.e. in several cases we expect stuff like this to be weighted:
space(weighting=n/10.0) # "randomly" gives no weighting for n=10
I guess here it matters less than for the discretized spaces, but in them it really maters.
spc = odl.uniform_discr(0, 5, n) # 'randomly' not weighted for n=5
this causes problems downstream for e.g. RayTransform
which has special behaviour for non-weighted spaces.
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'd still need a convincing argument why weighting with constant 1 would be fundamentally different from no weighting. I currently see it just as an optimization that scraps a multiplication with 1.
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.
Is this not a very weird behaviour:
>>> spc = odl.uniform_discr([0, 0], [9.999, 9.999], [10, 10])
>>> rt = odl.tomo.RayTransform(spc, odl.tomo.parallel_beam_geometry(spc))
>>> rt.range
uniform_discr([ 0. , -14.1407], [ 3.1416, 14.1407], (45, 31))
vs
>>> spc = odl.uniform_discr([0, 0], [10, 10], [10, 10])
>>> rt = odl.tomo.RayTransform(spc, odl.tomo.parallel_beam_geometry(spc))
>>> rt.range
uniform_discr([ 0. , -14.1421], [ 3.1416, 14.1421], (45, 31), weighting=1.0)
I mean, wat? this has to be considered a rather severe bug, no?
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.
That seems quite wrong.
But part of the problem seems to me that we need to jump through hoops currently to make the adjoint correct for weighted vs. unweighted spaces. Doing it via something like #1177 will solve the issue very elegantly by keeping all weighting stuff completely outside the operator definitions.
And when such a system is in place, no weighting and weighting with 1 will be exactly equivalent.
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.
Well this has nothing to do with the adjoint (in fact we don't even need the adjoint to have this problem), the problem here is that we want "the same weighting scheme" on the range as on the domain, but if we dont distinguish no weighting and 1.0 weight, we can't do that, because there is no concept of "the same weighting".
The solution in this case would be to fully remove the is_weighted
flag and remove the "unweighted" functionality for the ray transform, but I'm not sure if I'm happy about that.
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.
Oh right, I was totally forgetting about the fact that we infer the range. Okay now I got it. Hm. We do have an optional range
parameter for the ray transform, don't we? I tend to think that the behavior should be
- as currently when we have a
TensorSpace
withcell_volume
weighting, - no weighting in any other case.
To make it easier to change this, we could have a asweighted()
method on TensorSpace
where a new space of the same type but with new weighting can be created.
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.
Well it's not quite obvious to me how we would figure out if we're using cell_volume weighting 1.0 or no weighting unless we expose this flag somehow. We previously did it by exposing thetype of the weighting, but I guess we could fix this by exposing the argument used to create the weighting (somehow), i.e. space.weighting_type
returns a string 'const'
or w/e. Does that sound reasonable?
I feel adding yet another as_...
should hopefully not be needed right now, but I guess in the long run.
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.
Has this been further addressed? Otherwise we need to solve it soon (perhaps not in this PR tho)
odl/space/cupy_tensors.py
Outdated
raise RuntimeError("no conversion for dtype {}" | ||
"".format(arr.dtype)) | ||
else: | ||
space = type(self.space)(arr.shape, dtype=self.dtype, |
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.
weighting?
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'll port the current implementation in NumpyTensorSpace
, the proper solution will come with #1238
Just in general: if you squash axes, there's no obvious way to propagate weightings.
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.
We need to settle what squashed axes mean. Imo it should set that dimension to "1" so to say, i.e. we keep space[0:1].weighting
and space[0].weighting
different.
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.
Well, as of now, i.e., before #909 is done (part of #1238) we can't know how to handle weighting constants when removing axes. If all axes stay intact, we can keep the constant. If an axis is removed, we have to fall back to default. Not so great, but yeah.
For array weighting, i.e., the weights have the same shape as the space, we can just index them the same way, so that's a bit easier.
odl/space/cupy_tensors.py
Outdated
Numpy implementation is used which causes significant overhead | ||
due to data copies between host and device. | ||
""" | ||
# TODO: Test with some native ufuncs, then remove this attribute |
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.
fix this todo
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.
This raises a question: If we have a cupy space element and run some numpy code on it that requires a cast to numpy array, should we just go ahead and do it, at the cost of hidden performance loss, or should we raise?
I have a slight preference but want to make sure we're on the same page.
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.
Well as always im in the "at least it works" boat. I guess it comes down to how feature complete cupy is, if it has almost everything it is fine, but if you stumble every thing you do, it's gonna be irritating to work with
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'm in the same boat, so that's fine then. As far as I can see, they're pretty complete on the ufunc front, but methods like at
and accumulate
are largely not implemented. So we need some workaround to use the CUDA code for the obvious ones (sum
, prod
, cumsum
, cumprod
for add
and multiply
), but otherwise drop out to Numpy.
The cupy folks are a bit more worried about efficiency, probably since in neural nets, things are already buried deep down and hard to debug, so this kind of bottleneck danger would be pretty bad for them.
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.
Go ahead with that then!
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've implemented the manual fiddling.
odl/space/cupy_tensors.py
Outdated
newreal : `array-like` or scalar | ||
The new real part for this tensor. | ||
""" | ||
self.real.data[:] = newreal |
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 think self.data.real[:]
would be slightly more efficient?
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.
Very possible.
odl/space/cupy_tensors.py
Outdated
real : `CupyTensor` view with real dtype | ||
The real part of this tensor as an element of an `rn` space. | ||
""" | ||
# Only real dtypes currently |
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.
Quite sure there were complex ones above?
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.
Needs update obviously.
odl/space/cupy_tensors.py
Outdated
if np.isscalar(weights): | ||
weighting = CupyTensorSpaceConstWeighting(weights, exponent=exponent) | ||
else: | ||
# TODO: sequence of 1D array-likes |
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.
Cite the issue number
odl/space/cupy_tensors.py
Outdated
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
"""Implementation of tensor spaces using ``pygpu``.""" |
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.
Obviously the docs need update :-)
odl/space/cupy_tensors.py
Outdated
>>> same_space == space | ||
True | ||
""" | ||
return (super().__eq__(other) and |
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.
Update
Ok, so odlcuda is officially broken. I'll try to fix it, but getting this in would be a preferable solution. What is the ETA here, code looks quite good. |
I'll give it a couple of days of focused work, but those days will be distributed a bit. I give it high prio at least. |
I'm fixing odlcuda anyway |
I'll look into this today. Also in view of #1246. |
Getting there. I fixed the complex stuff, and all the doctests go green. Unit tests next. |
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.
Some further comments to take into account. This is looking great!
odl/space/cupy_tensors.py
Outdated
signature_string, indent) | ||
|
||
try: | ||
import cupy |
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.
How long does this import take?
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.
40 ms, it's really fast.
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.
Great! No worries then
odl/space/cupy_tensors.py
Outdated
return lico(a, x, 1, y, y) | ||
|
||
|
||
def _flat_inc(arr): |
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.
what does inc
mean here?
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's the incx
, incy
stuff needed for the cuBLAS functions.
odl/space/cupy_tensors.py
Outdated
|
||
|
||
def _flat_inc(arr): | ||
"""Compute the flat element stride for cuBLAS if possible, else raise.""" |
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.
A Parameters
, Returns
, Raises
here would be great
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 working on that this morning. The implementation was also not correct :-)
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 redid the cuBLAS handling, works as expected now. Also the lico
fallback kernel takes arrays of different dtypes.
odl/space/cupy_tensors.py
Outdated
Numpy implementation is used which causes significant overhead | ||
due to data copies between host and device. | ||
""" | ||
# TODO: Test with some native ufuncs, then remove this attribute |
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.
Go ahead with that then!
rn(3, impl='cupy', weighting=[1, 2, 3]) | ||
>>> space = odl.tensor_space((2, 3), impl='cupy', dtype=int) | ||
>>> space | ||
tensor_space((2, 3), 'int', impl='cupy') |
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.
Won't these doctests fail misserably for users without cupy, or do we exclude them somehow?
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.
At least not when running the file. I'll have to check how to configure pytest
to exclude them as well.
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.
Well if you explicitly run this file, you should get errors IMO, its harder with the global tests
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.
Done
odl/space/cupy_tensors.py
Outdated
@property | ||
def data_ptr(self): | ||
"""A raw pointer to the data container. | ||
|
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.
Returns
-------
data_ptr : int
to show the type of the returned value
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 prefer not to add Returns
to @property
docstrings since they do not "feel" like functions that return something. I'll add it somewhere else.
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.
Anything goes as long as the info is there!
|
||
Notes | ||
----- | ||
The element-by-element comparison is performed on the CPU, |
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.
Really? Why not write a ReductionKernel
for this? Should be rather simple
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.
Makes sense.
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.
Forgot to update the doc?
Edit: that was indeed the case. I fixed the doc
odl/space/cupy_tensors.py
Outdated
>>> x[::2] | ||
rn(3, impl='cupy').element([ 1., 3., 5.]) | ||
|
||
The returned views are writable, so modificatons alter the |
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.
Use "view"
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.
Also mention that this is not always the case (i guess?)
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.
Not for index arrays and boolean arrays, right.
odl/space/cupy_tensors.py
Outdated
raise RuntimeError("no conversion for dtype {}" | ||
"".format(arr.dtype)) | ||
else: | ||
space = type(self.space)(arr.shape, dtype=self.dtype, |
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.
We need to settle what squashed axes mean. Imo it should set that dimension to "1" so to say, i.e. we keep space[0:1].weighting
and space[0].weighting
different.
|
||
|
||
if CUPY_AVAILABLE: | ||
dotw = cupy.ReductionKernel(in_params='T x, T y, W w', |
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.
Are these compiled on the fly later? We don't want to create startup latency
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.
Yes, as I wrote somewhere else, the import time of this module is practically identical to the import time of cupy
, around 40 ms. So these guys don't add overhead at creation time.
tmp = np.empty(out.shape, out.dtype, order=out.space.default_order) | ||
with writable_array(out) as out_arr: | ||
tmp = array_module(impl).empty( | ||
out.shape, out.dtype, order=out.space.default_order) |
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.
This of course assumes that impl
has the same interface as numpy.
Perhaps it's better to implement wrappers for the most common functions, like asarray
(already done), empty
, and whatever else is needed. That would be more extensible.
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.
That would be how e.g. Keras does it, but I say we leave this for now. Changing should be rather easy (just search replace basically).
Array into which the result should be written. Must be contiguous | ||
and of the correct dtype. | ||
impl : str, optional | ||
Array backend for the output, used when ``out`` is not given. |
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.
Should probably mention where the valid choices come from.
@@ -387,7 +387,7 @@ def __pow__(self, shape): | |||
shape = tuple(shape) | |||
|
|||
pspace = self | |||
for n in shape: | |||
for n in reversed(shape): |
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.
This was an actual bug.
def asarray(self, out=None): | ||
"""Extract the data of this array as a ``numpy.ndarray``. | ||
def asarray(self, out=None, impl='numpy'): | ||
"""Extract the data of this array as an ndarray. |
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.
Perhaps make a glossary term ndarray
that explains the different flavors.
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.
Agree here
@@ -2266,7 +2288,7 @@ def norm(self, x): | |||
if self.exponent == 2.0: | |||
return float(np.sqrt(self.const) * _norm_default(x)) | |||
elif self.exponent == float('inf'): | |||
return float(self.const * _pnorm_default(x, self.exponent)) | |||
return float(_pnorm_default(x, self.exponent)) |
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've changed this because there are two reasons for it:
-
The mathematical one:
||x||_{p, w} --> ||x||_{inf, w}
should hold forp --> inf
(we knew that). -
Also structure-wise, all unweighted p-norms follow the pattern
norm = post_map( reduce( map(x) ) )
e.g.
1 <= p < inf
:map(x) = abs(x) ** p
,reduce(x) = sum(x)
,post_map(x) = x ** (1/p)
p = inf
:map(x) = abs(x)
,reduce(x) = max(x)
,post_map(x) = x
Weighted norms have the structure
norm = post_map( reduce( weight * map(x) ) )
and here it follows naturally that the weights play no role for
p = inf
since they do not change themax
. Why treat this case completely differently?
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 guess you're right, lets do it like this for now.
@@ -500,7 +500,7 @@ def array(self): | |||
|
|||
def is_valid(self): | |||
"""Return True if the array is a valid weight, i.e. positive.""" | |||
return np.all(np.greater(self.array, 0)) | |||
return (self.array > 0).all() |
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.
Again assuming a certain API of the arrays. Perhaps better to have wrapper all
, any
and such. Dunno.
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.
Lets leave it like this until someone wants to add something that breaks it
for name, nin, nout, docstring in UFUNCS: | ||
if nin == 2: | ||
# Currently not supported | ||
continue |
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.
We had a bunch of dysfunctional ufunc ops created here.
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.
Good catch
if isinstance(iter2, CupyTensor): | ||
iter2 = iter2.asarray() | ||
elif isinstance(iter2, cupy.ndarray): | ||
iter2 = cupy.asnumpy(iter2) |
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.
Should use utility.asarray
here I guess.
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.
yes
@@ -181,6 +203,14 @@ def is_subdict(subdict, dictionary): | |||
return all(item in dictionary.items() for item in subdict.items()) | |||
|
|||
|
|||
def xfail_if(condition, reason=''): | |||
"""Return a ``pytest.xfail`` object if ``condition`` is ``True``.""" |
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.
Explain usage.
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.
Massive review. Mostly looking good, just some minor stuff.
With that said, the increase in test run-time is worrying. I guess we can live with it for now, but we must diagnoize and improve the situation.
@@ -387,7 +387,7 @@ def __pow__(self, shape): | |||
shape = tuple(shape) | |||
|
|||
pspace = self | |||
for n in shape: | |||
for n in reversed(shape): |
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? Wouldn't our users expect r2 ** (3, 4) == (r2 ** 3) ** 4
?
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 don't think so. When doing space[i]
, the index slices along the outermost power, i.e., for space ** (3, 4)
you expect the valid indices i
to be 0, 1, 2
, no? It's the same logic as (R^m)^n == R^(n x m)
.
In your example, I would expect that r2 ** (3, 4)
has shape (3, 4, 2)
.
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 concur with your statement. It certainly makes sense.
be applied, a ``ValueError`` is raised, triggering a fallback | ||
implementation. | ||
|
||
For **1 array**, the conditions to be fulfilled are |
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.
array 1?
return _cublas_scal( | ||
x.data.device.cublas_handle, x.size, a, x.data.ptr, incx) | ||
|
||
scal.__name__ = scal.__qualname__ = '_cublas_scal' |
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 not simply name it _cublas_scal
?
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.
Renamed this.
|
||
If possible, a cuBLAS implementation is returned, otherwise a fallback. | ||
|
||
In general, cuBLAS requires single or double precision float or complex |
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.
This string is overly long for these, simply reference the function above.
This implementation is a highly optimized, considering all special | ||
cases of array alignment and special scalar values 0 and 1 separately. | ||
""" | ||
scal1 = _get_scal(x1.data) |
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.
Perhaps we should consider some type of cache for these? Or is that implemented downstream?
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.
Cache is an optimization that will have to come later. I'll focus on getting this in first.
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.
Agreed, we have the same thing on the Numpy spaces, we can add caches for both in one go.
for name, nin, nout, docstring in UFUNCS: | ||
if nin == 2: | ||
# Currently not supported | ||
continue |
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.
Good catch
@@ -436,6 +447,7 @@ def ufunc_factory(domain=RealNumbers()): | |||
globals()[name] = ufunc_factory | |||
__all__ += (name,) | |||
|
|||
np.seterr(**npy_err_old) |
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.
We should honestly add some "we're so sorry" line above this :D
if isinstance(iter2, CupyTensor): | ||
iter2 = iter2.asarray() | ||
elif isinstance(iter2, cupy.ndarray): | ||
iter2 = cupy.asnumpy(iter2) |
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.
yes
from odl.space.pspace import ProductSpace | ||
|
||
if isinstance(space, ProductSpace) and not space.is_power_space: | ||
raise ValueError('`space` cannot be a non-power product space') |
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 not? Part of the reason we use this function is that it supports these things (as compared to e.g. space.element(np.random.rand)
)
# Workaround for `shape` not using the base space shape of a | ||
# power space | ||
# TODO: remove when fixed, see | ||
# https://github.com/odlgroup/odl/pull/1152 |
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'll get this in ASAP
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.
This is now in, yay!
So, I'm officially announcing that I'll "take over" this PR and make a new PR for it. |
Go for it! Maybe close this one for difficulty to open the page? |
ValueError | ||
|
||
Slicing in the **fastest** axis is allowed as it results in a constant | ||
stride in the flat memory. Slicing with stride in any other axis is |
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.
Oops, "is" shouldn't be there
I'll close this once I've fixed the outstanding issues |
Any news on this front? |
How far is the integration process? Thanks in advance and best regards, Nikita |
This has sadly stalled quite badly and I will not have time to focus on it in the coming months. If anyway wants to pick up I'd be more than happy. |
Are there some tests or do you have a summary what needs to be done or
where one could start?
Am Do., 16. Mai 2019 um 17:13 Uhr schrieb Jonas Adler <
notifications@github.com>:
… This has sadly stalled quite badly and I will not have time to focus on it
in the coming months. If anyway wants to pick up I'd be more than happy.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1231?email_source=notifications&email_token=AAJ3R3JJGRZ526MMSBPVQD3PVV2Y7A5CNFSM4EDR6TCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVSEDPQ#issuecomment-493109694>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJ3R3OJQDNXRJL3K2G4WYTPVV2Y7ANCNFSM4EDR6TCA>
.
|
Sadly there is no summary but the main issue is to get the tests running. With that said this PR has become so old that it might be worth re-starting it with the newly updated spaces, perhaps @kohr-h knows how much has changes that touches upon this? |
No description provided.