-
Notifications
You must be signed in to change notification settings - Fork 103
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
Tensor spaces, second try #861
Conversation
f530983
to
8a6d51f
Compare
59b0936
to
ee061ea
Compare
Regarding this topic: I have implemented a simple indexing capability for spaces, but it is nowhere near as powerful and flexible as the lazy approach of simply indexing the array and appropriately wrap whatever comes out. arr = self.data[indices]
return self.space[indices].element(arr) is much much less powerful than lazy indexing, if we don't put great amounts of work into it. So I'd say the space indexing can be used for simple things like >>> space = odl.rn((2, 3, 4))
>>> space[[0, 2]]
rn((2, 4))
>>> space[0]
rn(2) but not for indexing of elements. |
I realized that indexing like this is not really helpful since it's not compatible with power spaces. I'll think about it a bit more. |
Is this in a state where it needs a review? If so I guess I need to get at it. |
Yes it is. I deliberately didn't mention it so nobody burns time at it that they don't have :-) |
Could you do the rebase from master first? It seems rather big. |
Yes, I've also spotted some glitches that I want to fix along the way. |
6a1dd6e
to
5292f10
Compare
Rebased |
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.
Huge review done, but mostly small things. I have not reviewed the tests yet, will do that later on.
On the larger scale, I have a few comments:
- The support for
DiscreteLp
with vector/tensor valued functions needs to be added and properly supported. This is the largest out-standing problem with this. - That all examples default to
shape=(2,3)
makes the doc much harder to read and adds little. I understand that during development this is important, but in release we should trim this in general, just like we dont usedtype='float32'
everywhere. - You have added
__hash__
in a few places where it shouldn't, i.e. in mutable objects. Also I think you may have missed adding__ne__
in some places, check that. - The
matrix_representation
/MatrixOperator
thing needs to be fixed, but could use a new issue.
|
||
x0 = np.arange(fn.size) | ||
x = fn.element(x0) | ||
x0 = np.arange(tspace.size) |
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.
tspace.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.
Haven't looked much into this file, probably needs more love
doc/source/dev/extend.rst
Outdated
Adding Fn spaces | ||
---------------- | ||
The abstract spaces `FnBase` and `NtuplesBase` are the workhorses of the ODL space machinery. They are used in both the discrete :math:`R^n` case, as well as data representation for discretized function spaces such as :math:`L^2([0, 1])` in the `DiscretizedSpace` class. These are in general created through the `rn` and `uniform_discr` functions who take an ``impl`` parameter, allowing users to select the backend to use. | ||
Adding Tensor 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.
Skipping the doc for now.
doc/source/generate_doc.py
Outdated
@@ -63,26 +63,26 @@ | |||
|
|||
|
|||
def import_submodules(package, name=None, recursive=True): | |||
""" Import all submodules of a module, recursively, including subpackages | |||
""" | |||
"""Recursively import all submodules of ``package``.""" |
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.
Improve. I guess it returns the names etc?
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.
will check
@@ -11,7 +11,7 @@ Glossary | |||
|
|||
array-like | |||
Any data structure which can be converted into a `numpy.ndarray` by the | |||
`numpy.array` constructor. Includes all `NtuplesBaseVector` based classes. | |||
`numpy.array` constructor. Includes all `Tensor` based classes. |
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 NtuplesBaseVector
-> Tensor
may be our best rename in a long time!
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.
Yeah scrap those shitty old names!
doc/source/guide/glossary.rst
Outdated
@@ -22,7 +22,7 @@ Glossary | |||
|
|||
discretization | |||
Structure to handle the mapping between abstract objects (e.g. functions) and concrete, finite realizations. | |||
It encompasses an abstract `Set`, a finite data container (`NtuplesBaseVector` in general) and the mappings between them, :term:`sampling` and :term:`interpolation`. | |||
It encompasses an abstract `Set`, a finite data container (`GeneralizedTensor` in general) and the mappings between them, :term:`sampling` and :term:`interpolation`. |
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.
Didn't we scrap GeneralizedTensor
?
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, needs to be replaced
odl/space/pspace.py
Outdated
return ProductSpace(*self.spaces[indices], field=self.field) | ||
elif isinstance(indices, tuple): | ||
if len(indices) > 1: | ||
raise ValueError('too many indices: {}'.format(indices)) |
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.
Another option is to pass these through further down, i.e.
return ProductSpace(self.spaces[indices[0]][indices[1:]], field=self.field)
odl/space/pspace.py
Outdated
@@ -848,7 +854,7 @@ def show(self, title=None, indices=None, **kwargs): | |||
-------- | |||
odl.discr.lp_discr.DiscreteLpElement.show : | |||
Display of a discretized function | |||
odl.space.base_ntuples.NtuplesBaseVector.show : | |||
odl.space.base_tensors.GeneralizedTensor.show : |
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.
find replace GeneralizedTensor
odl/space/space_utils.py
Outdated
impl : string, optional | ||
The backend to use. See `odl.space.entry_points.NTUPLES_IMPLS` and | ||
`odl.space.entry_points.FN_IMPLS` for available options. | ||
order : {'C', 'F'}, optional |
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'?
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.
Clearly missing
odl/space/space_utils.py
Outdated
`odl.space.entry_points.FN_IMPLS` for available options. | ||
order : {'C', 'F'}, optional | ||
Axis ordering of the data storage. | ||
impl : str |
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.
optional
odl/space/weighting.py
Outdated
if self.const <= 0: | ||
raise ValueError('expected positive constant, got {}' | ||
''.format(const)) | ||
if not np.isfinite(self.const): | ||
raise ValueError('`const` {} is invalid'.format(const)) | ||
|
||
def __hash__(self): |
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.
Prefer if __hash__
is always right after __eq__
since they are so strongly connected.
Thanks a lot for the detailed review @adler-j, will try to get this done asap. |
odl/space/npy_tensors.py
Outdated
"""Weighting of a `NumpyTensorSpace` with constant 1.""" | ||
|
||
# Implement singleton pattern for efficiency in the default case | ||
__instanc |
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 use a 3 dimensional array later.
Anyway, if we allow 2+, why not allow 1d as well, in which case this is an inner product.
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.
Wrong place again, you need to hack it apparently.
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 comment is w.r.t MatrixOperator
where you assume the matrix is 2 or higher dimensional. I can easily see this working with 1 or even 0 dimensional arrays, so that condition is arbitrary.
The line "You use a 3 dimensional array later." refers to a comment that MatrixOperator
input should be 2d, but you later use 3d. Also you later say that axis
needs to be 1 or more, but the default is 0
.
4fcd3a7
to
3183484
Compare
ebddf98
to
ac45d52
Compare
What is the review status on this? |
I figured that sampling would need tensor-valued functions, so I implemented them. Now I'm in the middle of writing/fixing the tests for those. |
Can you re-open this for a new review, if you want one, else we really want to go ahead here. |
After thinking about it some more, I think the following way of indexing spaces is most reasonable:
Opinions? |
The page for this PR has become unworkable once again, opening a new one. |
Copied from #753
TODOs:
NtuplesBase
,FnBase
,NumpyNtuples
andNumpyFn
in the corresponding tensor classesNtuples
and related stuffNtuples
and the likes as special case to tensor classesMatVecOperator
MatrixOperator
, see Reload modules in Spyder causes bugs #584. Solved in Issue 910 matvec op #911lp_discr
, ...)TensorSet
and others (see Scrap FunctionSet, DiscretizedSet and similar #860) duck-type space propertiesUse notion "rank" instead of "number of dimensions" to better distinguish betweenNot doing thisspace.ndim
andspace.size
TensorSpace
.Done, but kind of wrong, see here and hereDone properly nowWrite extra documentation on tensorsShould be part of Representing vector- and tensor-valued functions #908show
methods (1D fromNtuples
, 2D fromDiscreteLp
)FunctionSpaceElement
in 1D (again)FunctionSpace
as__getitem__
byaxis
attributeDiscreteLp
as inFunctionSpace
__array_ufunc__
interface onTensor
andNumpyTensor
(the latter only differs in weight propagation)__array_ufunc__
interface onDiscreteLpElement
__array_ufunc__
intefaces (bad input etc.)