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

ENH/TST: add __array_wrap__ to ProductSpaceElement #1288

Merged
merged 12 commits into from
Feb 19, 2018

Conversation

aringh
Copy link
Member

@aringh aringh commented Feb 12, 2018

To fix the observed issue in #1287.

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good

@adler-j
Copy link
Member

adler-j commented Feb 12, 2018

Lets merge this. For later, we need to fix the case of non-power spaces though. This should require the __array_ufunc__ interface.

Edit: product->power

@aringh
Copy link
Member Author

aringh commented Feb 12, 2018

The Doc build failed, but I don't understand why. Does anything need to be changed?

@aringh aringh force-pushed the add_array_wrap_to_product_space_element branch from aa70aaf to 5a8481e Compare February 13, 2018 09:04
@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

I just rebased on master, and with commit eb9e7d4 now in I hope that all tests should pass.

@adler-j
Copy link
Member

adler-j commented Feb 13, 2018

It seems the crash here is an actual bug and we should add ProductSpaceElement.real (and I guess imag and conj()).

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

That is what I am trying to figure out, except you are faster than me ;-)

In [1]: import odl
   ...: space = odl.rn(5)
   ...: pspace = odl.ProductSpace(space, 2)
   ...: x = space.one()
   ...: x_ps = pspace.one()
   ...: x.real
   ...: x_ps.real
Traceback (most recent call last):

  File "<ipython-input-1-140af8ed0ee3>", line 7, in <module>
    x_ps.real

AttributeError: 'ProductSpaceElement' object has no attribute 'real'

The weird thing is that the build did not fail before I rebased. But I do not see which commit in the last 24 hours that have changed this?

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

I had two test fails in this commit when I run it on my local machine. However, it is in a part of the code I do not think I have changed. We will see what Travis says...

Anyway, the test fails are:

=================================== FAILURES ===================================
____________________________ test_is_numeric_dtype _____________________________

    def test_is_numeric_dtype():
        for dtype in numeric_dtypes:
>           assert is_numeric_dtype(dtype)
E           AssertionError: assert False
E            +  where False = is_numeric_dtype(dtype('S'))

odl/test/util/utility_test.py:34: AssertionError
______________________________ test_is_real_dtype ______________________________

    def test_is_real_dtype():
        for dtype in real_dtypes:
>           assert is_real_dtype(dtype)
E           AssertionError: assert False
E            +  where False = is_real_dtype(dtype('S'))

odl/test/util/utility_test.py:39: AssertionError

Does @kohr-h or @adler-j know why these fail? Versions of some relevant packages used on my local machine:

numpy                     1.11.2                   py35_0
pytest                    3.2.1                    py35_0  
pytest-cache              1.0                      py35_0  
pytest-pep8               1.0.6                    py35_0

Copy link
Member

@kohr-h kohr-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally nice addition.

Question: what about setters? Shouldn't be too hard either, except if they should support broadcasting.

def real(self):
"""Pointwise real part of the element."""
size = self.space.shape[0]
to_return = [None] * size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this real_part instead of to_return?

@property
def real(self):
"""Pointwise real part of the element."""
size = self.space.shape[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(self) should do.

"""Pointwise real part of the element."""
size = self.space.shape[0]
to_return = [None] * size
for (i, part) in zip(range(size), self.__parts):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for i, part in enumerate(self.parts):

@@ -1081,6 +1081,32 @@ def ufuncs(self):
"""
return ProductSpaceUfuncs(self)

@property
def real(self):
"""Pointwise real part of the element."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointwise

@kohr-h
Copy link
Member

kohr-h commented Feb 13, 2018

@aringh Weird that those tests fail. These are the lines that generate the dtypes to be tested:

real_float_dtypes = np.sctypes['float']
complex_float_dtypes = np.sctypes['complex']
nonfloat_numeric_dtypes = np.sctypes['uint'] + np.sctypes['int']
numeric_dtypes = (real_float_dtypes + complex_float_dtypes +
nonfloat_numeric_dtypes)

I have no idea why np.dtype('S') ends up in there. Can you check manually what you get on your machine with your Numpy version?

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

I seem to have some extremely weird bug somewhere in my system... np.sctypes.keys() gives output
dict_keys(['float', 'int', 'uint', 'others', 'complex']), which looks fine. But then np.sctypes['complex'] gives output

 dtype('S'),
 dtype('<U'),
 dtype('S'),
 dtype('<U'),
 dtype('S'),
 dtype('<U'),
 dtype('S'),
 dtype('<U'),
 dtype('S'),
 ...]

We can ignore this. I will update numpy, and if it doesn't work I will simply create a new environment.

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this implementation is somewhat verbose, otherwise it should work

to_return = [None] * size
for (i, part) in zip(range(size), self.__parts):
to_return[i] = part.real
return self.space.element(to_return)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a problem since we want the real space, you need to implement ProductSpace.real_space and do

return self.space.real_space.element(to_return)

size = self.space.shape[0]
to_return = [None] * size
for (i, part) in zip(range(size), self.__parts):
to_return[i] = part.real
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats wrong with

to_return = [part.real for part in self.parts]

to_return = [None] * size
for (i, part) in zip(range(size), self.__parts):
to_return[i] = part.imag
return self.space.element(to_return)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See all points above

to_return = [None] * size
for (i, part) in zip(range(size), self.__parts):
to_return[i] = part.conj()
return self.space.element(to_return)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See all points above

x = noise_element(space)

# Test real
expected_result = space.element([np.real(x[0]), np.real(x[1])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use x[0].real instead

# Test conj. Note that ProductSpace does not implement asarray if
# is_power_space is false. Hence the construction below
expected_result = space.element([np.conj(x[0]), np.conj(x[1])])
assert all_almost_equal((x.conj())[0], expected_result[0])
Copy link
Member

@adler-j adler-j Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

x_conj = x.conj()
assert all_almost_equal(x_conj[0], expected_result[0])
assert all_almost_equal(x_conj[1], expected_result[1])

Edit @kohr-h: x.conj -> x_conj


# Test conj. Note that ProductSpace does not implement asarray if
# is_power_space is false. Hence the construction below
expected_result = space.element([np.conj(x[0]), np.conj(x[1])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use x[0].conj()

y = np.sin(x) # Should yield again an ODL product space element

assert all_equal(y, y_arr)
assert y in space
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this line to above the line before, since if y not in space we expect a fail, and this would be the natural order of testing it.

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

This took quite some time to get it correct... but I hope that it is ok now. However, I have therefore note added setters yet, as requested by @kohr-h. I will try to do that.

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

I think I would need some help: when implementing the setter for .real, should I care about if the element given to me have a non-zero imaginary part or not? Or should I just skip the imaginary part if it is none-zero?

@aringh
Copy link
Member Author

aringh commented Feb 13, 2018

First try for setter for real part. If possible, could @adler-j or @kohr-h review it before I do the imaginary part? I think you will have comments :-p, and the imaginary part is same-same-but-different.

Copy link
Member

@kohr-h kohr-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great that you take the effort to dig into this @aringh, kudos for that!
I've added some comments and my 2 cents on the setters.

if is_real_dtype(self.dtype):
return self
elif is_complex_floating_dtype(self.dtype):
new_dtype = TYPE_MAP_C2R.get(self.dtype, 'None')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the logic here? What should happen for 'None', i.e., an unsupported dtype? np.dtype will raise later on, but I think we should give a better error message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was indeed to just let astype raise the error after receiving None. But you are right, that might not be very helpful. I will raise a ValueError here (as well)

@@ -381,6 +384,83 @@ def dtype(self):
else:
raise AttributeError("`dtype`'s of subspaces not equal")

@property
def real_space(self):
"""The space corresponding to this space's real dtype.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where this comes from, but without the backticks this reads a bit cryptic. Maybe "Variant of this space with real dtype."

NotImplementedError
If `dtype` is not a numeric data type.
TypeError
If `dtype` is not a real or complex type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so happy with two different types of errors, both of which don't seem to be perfectly fit for the job. I'd say just make this a ValueError (because there is nothing wrong with the type in the Python sense).
I realize that this is inspired by base_tensors.py which also uses NotImplementedError, but in hindsight that wasn't a great choice either. IMO that should also be ValueError. Could you change that as well? There doesn't seem to be a test covering it 🙄 , so no tests should start failing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do some digging in the odl code to try to understand how this works, so indeed a lot of it is inspired or taken from base_tensor.py. I will do the change there as well.

TypeError
If `dtype` is not a real or complex type.
"""
if not is_numeric_dtype(self.dtype):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.dtype will fail for heterogeneous spaces. Since the error message from there wouldn't be obviously helpful, I'd suggest you catch the AttributeError and emit a nicer error here.

raise ValueError('`None` is not a valid data type')

dtype = np.dtype(dtype)
if dtype == self.dtype:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this self.dtype will fail for heterogeneous spaces, but here I think it would make sense to try astype on all parts anyway and not fail in that case. So this lookup needs to be guarded, for instance as getattr(self, 'dtype', object).

"""
if newreal in self.space.field:
for part in self.parts:
part.real.data[:] = newreal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use data, that's an implementation detail and not guaranteed to exist. Just go with part.real = ....

part.real.data[:] = newreal_subpart.real.data[:]
elif self.space.shape[0] == np.shape(newreal)[0]:
for part, newreal_subpart in zip(self.parts, newreal):
part.real.data[:] = np.real(newreal_subpart)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do all these checks, in particular don't extract the real part from the right-hand side. The backend will warn if such an assignment casts complex to float. IMO that's not quite enough since in most cases it's a sign of a bug, but we have to live with it. Taking the real part shadows such bugs.

So here just leave the RHS as it is.

raise ValueError('new real part does not the correct first '
'dimension, expected {} but got {}'
''.format(self.space.shape[0],
np.shape(newreal)[0]))
Copy link
Member

@kohr-h kohr-h Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the logic seems a bit too complicated for the cases it handles, and it misses out one case. Here are the cases that I think should be distinguished:

  1. RHS not iterable (likely a scalar) -> call each part.real setter with this RHS (don't check field or so)
  2. Space is a power space and RHS can be broadcast against the parts -> call each part.real setter with this RHS
    The reason for this (and that it comes before the next case) is that Numpy arrays behave the same: implicit new axes are added to the left, where leftmost is the power of the power space in our case:
    >>> a = np.zeros((2, 2))
    >>> a.real = [1, 2]
    >>> a
    array([[ 1.,  2.],
           [ 1.,  2.]])
    Note: This also needs to include element-like objects for the base space.
  3. len(rhs) == len(self) -> call part[i].real setter with rhs[i], i.e., pretty much the "standard" case.
  4. Error

So an implementation of that logic would be:

try:
    iter(newreal)
except TypeError:
    for part in self.parts:
        part.real = newreal
    return

if self.is_power_space:
    try:
        for part in self.parts:
            part.real = newreal
    except (ValueError, TypeError):  # maybe need to catch more, e.g. `LinearSpaceTypeError`
        pass
elif len(newreal) == len(self):
    for part, new_re in zip(self.parts, newreal):
        part.real = new_re
else:
    raise ValueError('invalid')  # better error

Edit: adapted logic in the middle to support broadcasting per part as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your logic is much cleaner than mine, no doubt about that ;-) However, it passes all the current tests except when newreal = [[0, 1 + 1j, 2], [3, 4]]. Now, that a test for setting the real part crashes when the element we try to set it to is complex might seem logical. But what I do not like is that this is the ONLY test with complex values where it crashes. All other tests pass (with warnings of course). Should we care about this inconsistency, or just be happy that it indeed actually captures one error correctly and not just gives warnings in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this implementation can have a quite laid-back attitude and just distribute work down in the way that makes most sense. That also means that raising errors is up to lower layers, so I wouldn't worry about that too much.
Regarding warnings vs. errors, we've been more and more going along with Numpy on most parts, simply because they probably have thought through a bunch of things and solved issues we don't even know about... and it's convenient! 😄

Anyway, what is this one error in the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails

import odl
space = odl.ProductSpace(odl.uniform_discr(0, 1, 3, dtype=complex), odl.cn(2))
x = odl.util.testutils.noise_element(space)
newreal = [[0, 1 + 1j, 2], [3, 4]]
x.real = newreal

and gives

Traceback (most recent call last):

  File "<ipython-input-3-ede4021b455c>", line 5, in <module>
    x.real = newreal

  File "[...]/odl/space/pspace.py", line 1249, in real
    part.real = new_re

  File "[...]/odl/discr/lp_discr.py", line 615, in real
    self.tensor.real = newreal

  File "[...]/odl/space/npy_tensors.py", line 1251, in real
    self.real.data[:] = newreal

TypeError: can't convert complex to float

while for example the following passes

import odl
space = odl.ProductSpace(odl.uniform_discr(0, 1, 3, dtype=complex), odl.cn(2))
x = odl.util.testutils.noise_element(space)
newreal = odl.util.testutils.noise_element(space)
x.real = newreal

Copy link
Member Author

@aringh aringh Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, just chaining to a power space makes the first code snippet pass:

import odl
space = odl.ProductSpace(odl.uniform_discr(0, 1, 3, dtype=complex), 2)
x = odl.util.testutils.noise_element(space)
newreal = [[0, 1 + 1j, 2], [3, 4, 5]]
x.real = newreal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me all 3 versions work, which may be a matter of Numpy version. So my conclusion is that we should just let Numpy (or whatever backend we use) do what it does, and unless there's a fundamental difference we don't add new behavior. That also means less work for you 😉

x.real = newreal
assert x.real == newreal.real

# Test for element in ProductSpace with none-zero imaginary part
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I wouldn't even test with complex right-hand sides. It's ultimately up to the backend (here Numpy) how to handle this, we don't really define ourselves what should happen in this case.

# Test that wrong first dimension do not pass
newreal = np.random.randn(5, 1) # space has 5 entries in total
with pytest.raises(ValueError):
x.real = newreal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you parametrize this test? It's a lot of repetition, I think it wouldn't be too hard to make a fixture for this.

Also, there's no test for the power space case with broadcasting yet, that would be an important one.

tmp = noise_element(space)
newreal = space.element(tmp.real)
x.real = newreal
assert x.real == newreal.real
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking ahead a bit: we've decided that we are going to become more like Numpy also for stuff like x == y, which means that code like this will break (see #1249).
I suggest that we proactively use all_equal for this case as well.

@aringh aringh force-pushed the add_array_wrap_to_product_space_element branch from c5705b4 to d1be7a5 Compare February 14, 2018 21:16
@aringh
Copy link
Member Author

aringh commented Feb 14, 2018

New try for the setter or real from ProductSpace. Also rebased on master. If this is (at least almost) good to go, I will make a copy-past implementation for .imag as well ;-)

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a proposal for simplification, else looks fine!

current_dtype = self.dtype
except(AttributeError):
raise ValueError(
'dtypes of underlying spaces are not the same and therefore '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, technically I could imagine that a space of mixed complex64 and complex128 spaces would yield a space of mixed float32/64.

Wouldn't a easier implementation of all of this be to remove all logic and simply do:

real_spaces = [spc.real_space for doc in self]
return ProductSpace(*real_spaces)

Or does the logic add something to this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right. I think this is what's called bootstrapping, no? :-)

if dtype == current_dtype:
return self
else:
return ProductSpace(*[space.astype(dtype)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what I meant above!

Copy link
Member Author

@aringh aringh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions to @kohr-h and @adler-j, but otherwise I hope that it should more or less be good to go

def imag(self, newimag):
"""Setter for the imaginary part.

This method is invoked by ``x.imag = other``.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, something like x.imag = 1j could to a user seem like a natural way of setting the imaginary part to 1. However, in this case it will be set to 0, since the imaginary part is a real number and thus set to the real part of 0 + 1j. Although logical mathematically, it might be confusing before you think about it. Any suggestion on how this can be clarified? Does it need to be clarified?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not need to be clarified since these are somewhat standard from e.g. numpy and other packages and hence easily googleable if anyone wants more info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should assume at least this basic amount of knowledge of complex numbers. I don't think x.imag = 1j should ever be expected to set the imaginary part to 1.

def real(self, newreal):
"""Setter for the real part.

This method is invoked by ``x.real = other``.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment below, should we mention what happens with the imaginary part of an element which one tries to set as real part, i.e., if someone does something like x.real = 1 + 1j?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it should throw an error. Is there a reason it does not? Have you tested what numpy does, since we want to do the exact same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>> arr = np.zeros(5)
>>> arr.imag = 3
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: array does not have imaginary part to set
>>> arr.real = 2j
__main__:1: ComplexWarning: Casting complex values to real discards the imaginary part

this is what we expect basically

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd even say that the exact behavior depends on the backend, and that we shouldn't add logic that interferes with that. So IMO we pass on whatever we get and let the error or warning bubble up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be more or less what is done, since setting the real part is more or less just delegated to the subspaces, so I will leave it as is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@aringh
Copy link
Member Author

aringh commented Feb 19, 2018

@kohr-h and @adler-j both still require changes ;-) Good to go?

Copy link
Member

@kohr-h kohr-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One bug needs fix, plus two minor things, otherwise LGTM


@property
def complex_space(self):
"""The space corresponding to this space's complex dtype."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use same formulation as in real_space

current_dtype = getattr(self, 'dtype', None)

if dtype == current_dtype:
return self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug here since

>>> np.dtype(float) == None
True

which means that self is returned if dtype == np.dtype(float) and no dtype of the current space can be determined. That's why I suggested object, not None, as getattr fallback (which assumes we will never support object).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is never exposed externally, prefer e.g. -1 as a fallback value since we know for sure we'll never support that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 raises an error that we would need to catch. Alternatively we could use np.void which is about the only thing that compares to False with np.dtype(object).

def __array_wrap__(self, array):
"""Return a new product space element wrapping the ``array``.

Only available if `is_power_space` is True.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

``True``
or alternatively ``is_power_space is True``

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still some minor stuff

from odl.util.ufuncs import ProductSpaceUfuncs
from odl.util.utility import TYPE_MAP_R2C, TYPE_MAP_C2R
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are no longer used

Examples
--------
>>> space = odl.ProductSpace(odl.cn(3), odl.cn(2))
>>> x = space.element([[1 + 1j, 2, 3 - 3j], [-1+2j, -2-3j]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer putting this on two lines for clarity

--------
>>> space = odl.ProductSpace(odl.cn(3), odl.cn(2))
>>> x = space.element([[1 + 1j, 2, 3 - 3j], [-1+2j, -2-3j]])
>>> x_real = x.real
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point it's probably more doctest like to simply write

>>> x.real
RESULT

same goes for imag

def real(self, newreal):
"""Setter for the real part.

This method is invoked by ``x.real = other``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

>>> zero = space.real_space.zero()
>>> x.real = zero
>>> x
space.element([[1j, 0j, -3j], [1j, 0]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to fail, right?

It should be something like

>>> x
ProductSpace(cn(3), cn(2)).element([
    [1j, 0j, -3j], 
    [1j, 0]
])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right... but it seems like doc-tests are not run for setters because it does not fail. And neither does

>>> False
True

I will try to have a look at it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is apparently a "feature" and all the documentation should go into the getter

https://stackoverflow.com/questions/23341106/python-2-7-doctests-ignored-in-setter-method-of-a-class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a general issue for this since we seem to have this error in many places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So apparently it seems like the doc-strings of .setter is ignored, see here. I will simply put the corresponding doc-test in the property itself, while keeping the rest of the documentation in the setter

>>> x_imag = x.imag
>>> real_space = odl.ProductSpace(odl.rn(3), odl.rn(2))
>>> expected_result = real_space.element([[1, 0, -3], [2, -3]])
>>> x_imag == expected_result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment in real


elem_params = ['space', 'real_space', 'numpy_array', 'array', 'scalar',
'1d_array']
elem_ids = [' form of element={} '.format(p) for p in elem_params]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too long, perhaps 'element' or something similarly short should make do?

newreal = np.random.randn()
elif element_form == '1d_array':
if not space.is_power_space:
pytest.skip('arrays mathcing only one dimension can only be used '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling error int "mathcing"

Copy link
Member

@adler-j adler-j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG for me!

@aringh aringh force-pushed the add_array_wrap_to_product_space_element branch from 06f7b84 to 48e2e4f Compare February 19, 2018 10:51
@aringh aringh merged commit 7b665e9 into odlgroup:master Feb 19, 2018
@aringh aringh deleted the add_array_wrap_to_product_space_element branch February 19, 2018 11:24
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

Successfully merging this pull request may close these issues.

3 participants