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

asarray() for power spaces #83

Closed
adler-j opened this issue Nov 25, 2015 · 14 comments
Closed

asarray() for power spaces #83

adler-j opened this issue Nov 25, 2015 · 14 comments

Comments

@adler-j
Copy link
Member

adler-j commented Nov 25, 2015

This could be implemented as calling asarray() on all subspaces, creating a N+1:d array. Another option is to create a linear array.

@kohr-h
Copy link
Member

kohr-h commented Nov 25, 2015

Or both, using an optional boolean flag contiguous or linear_arr to differentiate.

@adler-j
Copy link
Member Author

adler-j commented Nov 25, 2015

That is definitely a decent middle way.

@kohr-h
Copy link
Member

kohr-h commented Nov 25, 2015

Depends a bit on what one expects when asking a power space vector to become an array. Syntactically, one could index the vector with a single integer, vec[0], which then gave the component vector. The corresponding array would be the n-dim. one so you can use the same syntax arr[0] there to get the same as vec[0].asarray(). Hence, by default, that makes most sense IMO. An option (maybe called ravel or flat) to flatten the array would be good to have for certain applications, I guess.

@adler-j
Copy link
Member Author

adler-j commented Nov 25, 2015

I agree, but we should also note that the ndarray version only works for "PowerSpace" type spaces, what about unmatched pairs? ProductSpace(Rn(3), Rn(5))?

@kohr-h
Copy link
Member

kohr-h commented Nov 25, 2015

Correct, so then it's clear that only the flat version will fly. Any reason to make an optional n-dim array version for power spaces? If not, we don't differentiate.

@adler-j
Copy link
Member Author

adler-j commented Nov 25, 2015

Users could anyway themselves:

>>> x.asarray().reshape(...)

If they want nd version.

Another argument for the linear version is that we can make it recursive, so it will work on productspaces of productspaces. One interesting question is how we handle unmatched dtypes in this case, I suppose using numpy.find_common_type would make the most sense.

@kohr-h
Copy link
Member

kohr-h commented Nov 25, 2015

That's an interesting issue. Using the find_common_type sounds like a good idea. Tests will reveal if that's true.

@kohr-h
Copy link
Member

kohr-h commented Jan 23, 2017

This could be solved as part of #857 by going through the equivalent tensor-based power space.

@kohr-h kohr-h changed the title PowerSpace.asarray() asarray() for power spaces Jan 23, 2017
@adler-j
Copy link
Member Author

adler-j commented Jan 24, 2017

Well that is part of the problem, but what about

>>> pspace = odl.ProductSpace(odl.rn(5), odl.rn(6))
>>> array = pspace.zero().asarray()

what is array in this case?

@kohr-h
Copy link
Member

kohr-h commented Jan 24, 2017

Well, the issue was about power spaces from the beginning, so that's strictly speaking another issue.

Not sure what to do in the general case. Is a single array ever needed for general product spaces or is it rather so that different things will happen in different components anyway? Strongly depends on the set of use cases.

@adler-j
Copy link
Member Author

adler-j commented Jan 24, 2017

Well any user would expect some form of continuity, i.e if we make different choices we would get weird situations like

>>> pspace = odl.ProductSpace(odl.rn(2), odl.rn(3))
>>> array = pspace.zero().asarray().shape
(5,)
>>> pspace = odl.ProductSpace(odl.rn(3), odl.rn(3))
>>> array = pspace.zero().asarray().shape
(2, 3)

My suggestion would be to leave the "shape preserving case" for #857, and let ProductSpace give something that works in all cases, which to me seems like a long flat array. Another option is to simply not allow calling asarray unless the result has a well defined shape.

@kohr-h
Copy link
Member

kohr-h commented Jan 24, 2017

That's true, so then we need to settle on the least common denominator for this case, which would probably be the linear array. Or not implementing asarray at all for ProductSpace.
In the more common use case it is

>>> pspace = odl.rn(3) ** 2
>>> pspace
odl.rn((2, 3))
>>> pspace.asarray().shape
(2, 3)

@adler-j
Copy link
Member Author

adler-j commented May 23, 2017

I've actually implemented this in #972, where the implementation is to make it a nd-array (i.e. asarray does NOT work with non-powerspaces) I felt this was the best way to do stuff given that tensors are comming.

Further, the "make linear array" would have very few use-cases, most of which get over-ridden by __numpy_ufunc__ finally coming.

@adler-j adler-j mentioned this issue Nov 14, 2017
20 tasks
@kohr-h
Copy link
Member

kohr-h commented Feb 15, 2018

This has been done in #1139.

@kohr-h kohr-h closed this as completed Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants