Op repr updates by kohr-h · Pull Request #1394 · odlgroup/odl

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

        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

@@ -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

+        )
+        """
        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

 """

-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

+        --------
+        >>> 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

+        >>> 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

+
+        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

+             [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

+        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

        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

                                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

+        >>> 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

        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 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 status: obsolete topic: documentation type: enhancement labels Apr 13, 2020

Conversation

kohr-h commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohr-h commented Aug 22, 2018

Uh oh!

adler-j commented Aug 23, 2018

Uh oh!

adler-j left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohr-h Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adler-j Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohr-h commented Aug 31, 2018

Uh oh!

kohr-h commented Apr 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kohr-h commented Jun 25, 2018 •

edited

Loading

pep8speaks commented Jun 25, 2018 •

edited

Loading

kohr-h Aug 26, 2018 •

edited

Loading

adler-j Aug 27, 2018 •

edited

Loading