Op repr updates #1394

kohr-h · 2018-06-25T20:37:48Z

~~(I just realized that this is based on the #1380 branch, so that should go in first.)~~
Ready for review now.

Here's the next bunch of commits from #1276, kept to the minimum of space, discr and operator. Other submodules follow.

Biggest changes:

__repr__ makes use of the new functionality with automatic line width adjustment (using np.get_printoptions()['linewidth']).
All __repr__ methods now have a doctest, to make sure the code runs and does something useful at least in one case.
Several operators that lacked custom range got it now.
Several operators changed from ...Base with common children to inner class pattern to share context.

Questionable stuff:

Spaces have asweighted, not quite sure about this
Some optimization of operator arithmetic, any problems?
Attribute repr doesn't quite work that well with linewidth. The reason is that it calls repr(parent_op) without knowing about the correct linewidth to use, and then appends its attribute string. I don't quite know how to solve this, but it doesn't feel like a pressing issue either.
To implement custom (e.g. weighted) range for product space operators (e.g., PointwiseInner), we need to consider spaces as compatible (in this narrow context) that have different weightings but are otherwise the same. Needs some thought about the consequences though.

Additional changes/fixes:

Add Numpy 1.14 to Travis
Travis setup with Numpy versions wasn't ideal, it would always take the 1.*.0 version, but 1.14.0 is broken, for instance. Changed to take the latest compatible.
Exclude Numpy 1.14.0 to 1.14.2 due to printing bugs, messes up doctests and such. Easy to get newer version
Product space elements are printed slightly more compactly
Adapted TODO comments
Default print of Operator is better now IMO
Removed separate __str__ mostly, doesn't really add value very often. The base Operator class defaults to __str__ = __repr__ (not literally)
Imports now sorted with isort. Also added an .isort.cfg.

TODOs (after review):

Make repr precision controllable by user (look at numpy.getprintoptions()['precision']
Remove .isort.cfg
Pull out the operator arithmetic stuff ~~into a separate PR~~ to /dev/null

pep8speaks · 2018-06-25T20:38:04Z

Checking updated PR...

In the file odl/operator/tensor_ops.py, following are the PEP8 issues :

Line 549:80: E501 line too long (80 > 79 characters)

Complete extra results for this file :

    ProductSpace(uniform_discr([-1., -1.], [ 1.,  1.], (1, 2)), 2).element([                                                                               ^---

Comment last updated on August 30, 2018 at 21:51 Hours UTC

kohr-h · 2018-08-22T15:17:45Z

odl/discr/diff_ops.py

+            [[  2.,   2.,   2.,   2.,  -3.],
+             [  2.,   2.,   2.,   2.,  -4.],
+             [ -1.,  -2.,  -3.,  -4., -12.]]
+        )

        Verify adjoint:

        >>> g = div.range.element(data ** 2)
        >>> adj_div_g = div.adjoint(g)


Remove line

kohr-h · 2018-08-22T15:24:51Z

Hey, could someone please have a look at this?

It may seem like a daunting task with so many lines of diff, but it's actually not that much. Most of it is documentation and doesn't need that close of a look, in the sense that errors that slip through won't break anything.

I've listed the most important changes above.

adler-j · 2018-08-23T12:21:59Z

I'll try to review this!

adler-j

Overall good changes, but this really should have been 2 or even 3 different pull requests since it does wastly different things.

The only thing that needs discussion is the general addition of ranges where it does not seem obviously needed. Why has this been done?

adler-j · 2018-08-23T12:24:07Z

.isort.cfg

@@ -0,0 +1,14 @@
+[settings]


More spam of the top level directory. Why do we need this?

That file slipped through, I'll ditch it.

adler-j · 2018-08-23T12:49:14Z

odl/discr/diff_ops.py

        posargs = [self.domain]
-        optargs = [('range', self.range, self.domain ** self.domain.ndim),


Was this a misstake?

That was a bug, yes. Copy 'n paste from Gradient I suppose.

adler-j · 2018-08-23T12:49:54Z

odl/discr/discr_mappings.py

@@ -12,20 +12,21 @@
 operators.
 """

-from __future__ import print_function, division, absolute_import
+from __future__ import absolute_import, division, print_function


Is this just standardization? If so (for next time), I'd prefer if it had a separate PR and was done to all of these occurances.

Yeah I ran isort on the imports, so I don't have to think about where to put new imports. They'll be sorted and grouped nicely automatically.
I already did a few of the files in #1380, this part of the big PR #1276 just continues that. IMO the structure of the imports is better this way, compared to the "random" ordering we had before.

adler-j · 2018-08-23T13:08:20Z

odl/discr/discr_mappings.py

+        --------
+        >>> rect = odl.IntervalProd([0, 0], [1, 1])
+        >>> fspace = odl.FunctionSpace(rect)
+        >>> part = odl.uniform_partition_fromintv(rect, shape=(4, 2))


Do we need to use this very length version instead of the shorter options?

Maybe not...

adler-j · 2018-08-23T13:09:50Z

odl/discr/discr_ops.py

+        >>> resampling_inv(resampling([0.0, 1.0, 0.0]))
+        uniform_discr(0.0, 1.0, 3).element([ 0.,  1.,  0.])
+
+        However, it can fail in the other direction:


Extremely nice doc!

adler-j · 2018-08-23T14:25:19Z

odl/operator/pspace_ops.py

+
+        Examples
+        --------
+        All rows and columns with an operator, i.e., domain and range


Wording could be improved. E.g. use something else that "with an operator", perhaps even drop it and only go with "Domain and range are inferred if there is at least one operator per row/column", or similar.

adler-j · 2018-08-23T14:25:35Z

odl/operator/pspace_ops.py

+             [0, IdentityOperator(rn(3))]]
+        )
+
+        Domain not completely determined (column with all zeros):


Mention that we need to give domain in this case.

adler-j · 2018-08-23T14:25:41Z

odl/operator/pspace_ops.py

+        Domain not completely determined (column with all zeros):
+
+        >>> prod_op = odl.ProductSpaceOperator([[I, 0],
+        ...                                     [I, 0]], domain=pspace)


Push domain to next row

adler-j · 2018-08-23T14:32:39Z

odl/operator/pspace_ops.py

@@ -655,66 +672,70 @@ def __init__(self, space, index):

        Projection on the 0-th component:

-        >>> proj_adj = odl.ComponentProjectionAdjoint(pspace, 0)
+        >>> proj_adj = odl.ComponentProjection(pspace, 0).adjoint


Is there a reason we think users might not want this "embeding"? I'm not 100% sure we only want this accessible like this.

I didn't really think about that. My assumption, given the name, was that it was just there as a "slave" to the "master" class ComponentProjection.
The class may certainly be of use on its own, if I think about it. How about we call it ComponentEmbedding? (It feels like we discussed this before.)

adler-j · 2018-08-23T16:22:46Z

odl/space/base_tensors.py

@@ -427,7 +439,8 @@ def examples(self):
                   self.element(np.random.uniform(size=self.shape) +
                                np.random.uniform(size=self.shape) * 1j))
        else:
-            # TODO: return something that always works, like zeros or ones?
+            # TODO(kohr-h): return something that always works, like zeros


For non-numeric dtypes there is basically nothing left. What would zeros be for e.g. strings?

I think zeros actually works with any type, meaningful or not.

>>> np.zeros(2, dtype=bool) array([False, False], dtype=bool) >>> np.zeros(2, dtype="U2") array(['', ''], dtype='<U2') >>> np.ones(2, dtype=bool) array([ True, True], dtype=bool) >>> np.ones(2, dtype="U2") array(['1', '1'], dtype='<U2')

I'll be damned

>>> np.zeros(2, dtype=object) array([0, 0], dtype=object)

Yeah I think that one at least makes a bit of sense. Doesn't matter for us since we don't allow dtype=object. I personally find np.ones with string type hilarious.

kohr-h · 2018-08-26T21:04:18Z

odl/discr/discr_ops.py

+        >>> resampling = odl.Resampling(coarse_discr, fine_discr)
+        >>> resampling_inv = resampling.inverse
+
+        The inverse is proper left inverse if the resampling goes from a


slight wording issue

kohr-h · 2018-08-26T21:40:09Z

odl/space/base_tensors.py

@@ -258,6 +259,21 @@ def astype(self, dtype):
        else:
            return self._astype(dtype)

+    def asweighted(self, weighting):


How about this? It's going to be needed at some point, but not strictly here.

Another idea with this functionality would be to stuff it inside astype in kwargs, and to extend that function with more stuff that we might want to change. For instance interpolation, impl, or even trivial stuff like axis labels.

Bumping this

…ion.adjoint

kohr-h · 2018-08-31T14:42:32Z

This is largely done, up to some open questions.

kohr-h · 2020-04-13T10:19:59Z

I marked this as obsolete. It can be considered as a cherry-pick-from PR. So keeping it open until the changes are implemented some other way.

kohr-h force-pushed the op_repr_updates branch from 53f5983 to 6bfd886 Compare June 25, 2018 21:25

kohr-h added this to Needs review in PR Status Jun 28, 2018

kohr-h commented Aug 22, 2018

View reviewed changes

adler-j requested changes Aug 23, 2018

View reviewed changes

kohr-h commented Aug 26, 2018

View reviewed changes

kohr-h force-pushed the op_repr_updates branch 2 times, most recently from ac9aaa8 to 67f5b17 Compare August 26, 2018 22:11

kohr-h added 6 commits August 29, 2018 22:35

ENH: update repr and docs of operator and space

84cec7e

ENH: fix discr repr and operator tests

8bf750a

STY: pep8 fixes

25cedfb

DOC/TST: fix various doctest failures

9efe5ac

ENH: make repr precision more flexible

c80607c

MAINT: remove operator scaling and lico

958e0a0

kohr-h force-pushed the op_repr_updates branch from 67f5b17 to 958e0a0 Compare August 29, 2018 20:36

kohr-h added 3 commits August 29, 2018 23:36

MAINT: improve some docs and fix minor issues

f309694

MAINT: make standalone ComponentEmbedding class from ComponentProject…

7aa2b31

…ion.adjoint

BUG: fix wrong range of MultiplyOperator

e9c807c

adler-j mentioned this pull request Sep 11, 2018

Release 1.0.0 #1055

Open

20 tasks

kohr-h added this to In Progress in Release 1.0.0 Sep 11, 2018

kohr-h moved this from Needs review to Work in progress in PR Status Jul 18, 2019

kohr-h added status: obsolete topic: documentation type: enhancement labels Apr 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Op repr updates #1394

Op repr updates #1394

kohr-h commented Jun 25, 2018 •

edited

pep8speaks commented Jun 25, 2018 •

edited

kohr-h Aug 22, 2018

kohr-h commented Aug 22, 2018

adler-j commented Aug 23, 2018

adler-j left a comment

adler-j Aug 23, 2018

kohr-h Aug 26, 2018

adler-j Aug 23, 2018

kohr-h Aug 26, 2018

adler-j Aug 23, 2018

kohr-h Aug 26, 2018

adler-j Aug 23, 2018

kohr-h Aug 26, 2018

adler-j Aug 23, 2018

adler-j Aug 23, 2018

adler-j Aug 23, 2018

adler-j Aug 23, 2018

adler-j Aug 23, 2018

kohr-h Aug 26, 2018

adler-j Aug 23, 2018

kohr-h Aug 26, 2018 •

edited

adler-j Aug 27, 2018 •

edited

kohr-h Aug 27, 2018

kohr-h Aug 26, 2018

kohr-h Aug 26, 2018

kohr-h Aug 29, 2018

kohr-h commented Aug 31, 2018

kohr-h commented Apr 13, 2020

		posargs = [self.domain]
		optargs = [('range', self.range, self.domain ** self.domain.ndim),

Op repr updates #1394

Are you sure you want to change the base?

Op repr updates #1394

Conversation

kohr-h commented Jun 25, 2018 • edited

pep8speaks commented Jun 25, 2018 • edited

Comment last updated on August 30, 2018 at 21:51 Hours UTC

Choose a reason for hiding this comment

kohr-h commented Aug 22, 2018

adler-j commented Aug 23, 2018

adler-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h Aug 26, 2018 • edited

Choose a reason for hiding this comment

adler-j Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h commented Aug 31, 2018

kohr-h commented Apr 13, 2020

kohr-h commented Jun 25, 2018 •

edited

pep8speaks commented Jun 25, 2018 •

edited

kohr-h Aug 26, 2018 •

edited

adler-j Aug 27, 2018 •

edited