MAINT: rename TensorGrid->RectGrid and remove RegularGrid #841

kohr-h · 2017-01-20T17:57:13Z

Closes #670.

Also some maintenance changes regarding the ideg and inondeg properties, now replaced by a single nondegen_byaxis property that returns a full boolean array.

kohr-h · 2017-01-20T17:57:55Z

odl/discr/partition.py

-            [5.0],
-            [2.0],
-            min_pt=[-1.0, 1.0, 4.0, 2.0], max_pt=[-0.5, 3.0, 5.0, 3.0]
-        )


This now became a uniform partition and printed out an enormous line, had to remove it.

I dont get it, why did it change and how does it look now?

Now by definition, any axis with two cells or less is uniform, which is kind of hard to argue against. The printed return value was roughly

uniform_partition([-1.0, 1.0, 4.0, 2.0], [-1.0, 1.0, 4.0, 2.0], (1, 1, 1, 1), nodes_on_bdry=((True, False), False, (False, True), (True, False))

modulo error in the booleans.

I see why that was removed.

kohr-h · 2017-01-20T17:59:05Z

odl/test/tomo/backends/astra_setup_test.py

-    dpart_0 = odl.RectPartition(odl.IntervalProd(0, 0), odl.TensorGrid([0]))
-    geom_p2d = odl.tomo.Parallel2dGeometry(apart, dpart=dpart_0)
-    with pytest.raises(ValueError):
-        odl.tomo.astra_projection_geometry(geom_p2d)


This check no longer made sense since the grid was in fact uniform, hence the error condition was gone.

Well, perhaps we should change the grid then?

Too lazy :-)
Okay I'll do it. I had to get it out because gates close at 7, didn't want to be locked in.

adler-j

Great change. Minor review and go ahead.

adler-j · 2017-01-20T18:04:37Z

odl/discr/grid.py

+        .. math::
+            G = \{(-1, 0), (-1, 1), (0, 0), (0, 1), (2, 0), (2, 1)\}
+
+        Here is a graphical representation::


Does this typeset well?

I think it looks decent. It's treated as code, so it uses a monospace font.

adler-j · 2017-01-20T18:06:05Z

odl/discr/grid.py

+        if not self.is_uniform:
+            raise NotImplementedError('`stride` not defined for non-uniform '
+                                      'grid')
+        strd = np.zeros(self.ndim)


might as well write out stride instead of strd

Hesitant to shadow the function name, feels wrong.

The function name isn't shadowed by it since it wasn't available in the first place? The only way to access it is via self.stride

Okay, "shadow" was the wrong term, "simultaneously highlighted by the editor" would be more precise.

I guess editor confusion is also a reason, stay with this then.

adler-j · 2017-01-20T18:06:41Z

odl/discr/grid.py

+    @property
+    def stride(self):
+        """Step per axis between neighboring points of a uniform grid.
+


adler-j · 2017-01-20T18:08:29Z

odl/discr/grid.py

        True
        """
        # Optimization for some common cases
        if other is self:
            return True
-        if not (isinstance(other, TensorGrid) and
+        if not (isinstance(other, RectGrid) and


Why is this if statement still there?

To return False for wrong type instead of raising an AttributeError?

well any non rect grid should be false, I was thinking about the following lines but perhaps I missed the logic (hard to read with changelogs)

It would probably be easier to read as

if not isinstance(other, RectGrid) or not ...

It's easy to mismatch parens I guess.

Well

if not isinstance(other, RectGrid): return False # other logic here

is our standard pattern.

adler-j · 2017-01-20T18:09:38Z

odl/discr/grid.py

+                # vec_s. If there is no almost zero entry in each row,
+                # return False.
+                vec_o_mg, vec_s_mg = sparse_meshgrid(vec_o, vec_s)
+                if not np.all(np.any(np.abs(vec_s_mg - vec_o_mg) <= atol,


np allclose?

Oh well yeah, that was some old code I just copied.

No, allclose is not suitable since the two vectors are broadcast and form a matrix (like a distance matrix). Then it is checked that in each column there is at least one entry close enough to zero.
AFAICS the only thing I can do is replace the boolean abs expression with np.isclose.

adler-j · 2017-01-20T18:13:40Z

odl/discr/grid.py

-
-    def insert(self, index, other):
-        """Insert another regular grid before the given index.
+def uniform_grid(min_pt, max_pt, shape):


Good addition.

adler-j · 2017-01-20T18:14:28Z

odl/discr/grid.py

-        return '{}({})'.format(self.__class__.__name__, inner_str)
+    coord_vecs = [np.linspace(mi, ma, num, endpoint=True, dtype=np.float64)
+                  for mi, ma, num in zip(min_pt, max_pt, shape)]
+    return RectGrid(*coord_vecs)


 def uniform_sampling_fromintv(intv_prod, shape, nodes_on_bdry=True):


uniform_grid_fromintv now I guess

Better name eh? I'll change it.

adler-j · 2017-01-20T18:15:12Z

odl/discr/grid.py

@@ -1264,7 +1168,7 @@ def uniform_sampling_fromintv(intv_prod, shape, nodes_on_bdry=True):
            gmin.append(xmin + (xmax - xmin) / (2 * n))
            gmax.append(xmax - (xmax - xmin) / (2 * n))

-    return RegularGrid(gmin, gmax, shape)
+    return uniform_grid(gmin, gmax, shape)


 def uniform_sampling(min_pt, max_pt, shape, nodes_on_bdry=True):


How does this compare to uniform_grid?

This uses the interval product min/max, while the other uses the grid min/max directly. Subtle difference.

Oh god you're right, but with nodes_on_bdry=True they are equal, no?

Oh yes, of course. So perhaps one of them can go. I'll look into it.

adler-j · 2017-01-20T18:16:16Z

odl/discr/partition.py

-            [5.0],
-            [2.0],
-            min_pt=[-1.0, 1.0, 4.0, 2.0], max_pt=[-0.5, 3.0, 5.0, 3.0]
-        )


I dont get it, why did it change and how does it look now?

adler-j · 2017-01-20T18:24:27Z

odl/test/tomo/backends/astra_setup_test.py

-    dpart_0 = odl.RectPartition(odl.IntervalProd(0, 0), odl.TensorGrid([0]))
-    geom_p2d = odl.tomo.Parallel2dGeometry(apart, dpart=dpart_0)
-    with pytest.raises(ValueError):
-        odl.tomo.astra_projection_geometry(geom_p2d)


Well, perhaps we should change the grid then?

kohr-h · 2017-01-21T19:47:14Z

@adler-j you may want to look at 670864e and check if that's okay. I find it nice because it's now the package that solely defines the version.

adler-j

Minor comments, go ahead after.

adler-j · 2017-01-22T13:40:55Z

odl/__init__.py

 """

 from __future__ import absolute_import

 __version__ = '0.5.4.dev0'
-__all__ = ('diagnostics', 'discr', 'operator', 'set', 'space', 'solvers',
-           'tomo', 'trafos', 'util', 'phantom', 'deform', 'ufunc_ops')
+__all__ = ()


Are we sure all of them should be removed? How about stuff like util that is not default imported?

As I understand it, the imports further down put all the subpackages in the module dictionary, so that's not the purpose of adding them to __all__ anyway. The __all__ here is only effective if someone does from odl import *, and then I guess we don't want the subpackages themselves to be included, only their __all__ contents. Or do we?

Well if someone does from odl import all I guess they should get something called util? Otherwise they cant access everything. For subpackages that are included via import *, we don't need to import them specifically.

Hm, right. But that basically means that everything has to stay, since the same applies to the other subpackages. Currently you get everything from the subpackages' __all__ but the other stuff is not accessible, at least not directly.

That also means that we have to be careful not to include any module or package into __all__ that conflicts with another name somewhere in the library, or at least make sure that they're not imported into the same namespace.

adler-j · 2017-01-22T13:41:30Z

odl/discr/grid.py

+    the product of these lengths. This class makes use of that
+    sparse storage scheme.
+
+    See ``Notes`` for details.


Where did notes go?

Edit: Into Init, i'd keep it at class level.

But then in IDEs you get class docstring -> Notes -> Parameters, which IMO is not the best possible order. As I've argued earlier, and that's of course debatable, I think parameters should come up as quickly as possible, with all the extra stuff afterwards. That's why I moved the notes into __init__ after parameters and examples.

Still not convinced that we should change the logical structure of the doc in order to please a specific IDE typesetting, other IDEs may do this "properly" (i.e. distinguish between a class and its initializer in a better way than spyder). Very few will read this anyway.

To continue on the above, this is mostly dev doc and for developers those notes are very important, more so than the init doc.

That may be true, but how often is that important compared to the parameter explanations?

The math notes are important to understand what this represents, and that's usually when you read the doc for the first time. After that, you perhaps occasionally check the math but not often.

On the other hand, the parameters are important on a day-to-day basis when you use the function or class in practice. Of course the signature is printed first (usually, sometimes it doesn't work but that's nothing to count in), but when there are kwargs or if you need details about the parameters you need that section. And then at least I find it annoying if there are too many other things in the way that make me scroll every time I need info about the arguments.

So even from a dev perspective I feel that in the long run, parameters are more important than other details in the notes.

other IDEs may do this "properly" (i.e. distinguish between a class and its initializer in a better way than spyder)

Not sure what a better way would look like, or how other IDEs handle this. To me it seems reasonable to show the class docstring and the initialization doc if a user asks for documentation on a class.
The alternative would be to let the user choose, with pop-up menus like (apparently) in PyCharm. I personally find that annoying.

Made a new issue #854.

adler-j · 2017-01-22T13:43:42Z

odl/discr/grid.py

+        # Uniformity, setting True in degenerate axes
+        diffs = [np.diff(v) for v in self.coord_vectors]
+        self.__is_uniform_byaxis = tuple(
+            True if diff.size == 0 else np.allclose(diff, diff[0])


diff.size == 0 or np.allclose(diff, diff[0])

adler-j · 2017-01-22T13:44:49Z

odl/discr/grid.py

+        >>> rg = uniform_grid([-1.5, -1], [-0.5, 3], (2, 3))
+        >>> rg.stride
+        array([ 1.,  2.])
+


remove blankline

adler-j · 2017-01-22T13:46:08Z

odl/discr/grid.py

-                np.all(self.max_pt <= other.max_pt + atol)):
+        if not isinstance(other, RectGrid):
+            return False
+        if not(np.all(self.shape <= other.shape) and


little reason to use np.all for length ~3 arrays, more readable with simply all (and probably faster for this small case)

It's also wrong since the statement in parenthesis is already a boolean (compares tuples).

adler-j · 2017-01-22T13:48:59Z

odl/discr/partition.py

        IntervalProd.squeeze
        """
-        newset = self.set[self.grid.inondeg]
+        nondegen_indcs = np.where(self.grid.nondegen_byaxis)[0]


np.flatnonzero to skip the [0]

adler-j · 2017-01-22T13:50:47Z

odl/set/domain.py

@@ -102,7 +101,7 @@ def ndim(self):
    @property
    def true_ndim(self):
        """Number of non-degenerate (positive-length) intervals."""
-        return len(self.inondeg)
+        return sum(self.nondegen_byaxis.astype(int))


return np.count_nonzero(self.nondegen_byaxis)

adler-j · 2017-01-22T13:52:55Z

odl/set/domain.py

@@ -431,7 +432,7 @@ def measure(self, ndim=None):
        elif ndim > self.true_ndim:
            return 0.0
        else:
-            return np.prod((self.max_pt - self.min_pt)[self.inondeg])
+            return np.prod(self.extent()[self.nondegen_byaxis])


Remind me, why is extent not a property?

Probably because the thought was that for some grid or domain structures in the future, this might be costly to compute. If we're sure that it's always gonna be cheap, we can make it a property (but then what to do with min and max which are methods, too?).

min and max are methods in order to conform to the numpy interface, which allows these to be used with numpy in a duck-typing style. Otherwise we provide min_pt etc which do the same.

I'll propose a change.

Issue: #847

- Similarly uniform_sampling_fromintv->uniform_grid_fromintv - Remove old uniform_grid

kohr-h commented Jan 20, 2017

View reviewed changes

adler-j approved these changes Jan 20, 2017

View reviewed changes

adler-j mentioned this pull request Jan 20, 2017

Some objects that define __eq__ should define __hash__ #843

Closed

kohr-h mentioned this pull request Jan 21, 2017

Update numpydoc submodule #845

Closed

kohr-h force-pushed the issue-670__rect_grid branch 2 times, most recently from 5ca2195 to 61a29db Compare January 21, 2017 19:58

adler-j approved these changes Jan 22, 2017

View reviewed changes

adler-j mentioned this pull request Jan 22, 2017

Change extent() to a property #847

Closed

Holger Kohr added 5 commits January 22, 2017 19:34

MAINT: rename TensorGrid->RectGrid and remove RegularGrid

a551190

DOC: replace old text in __init__.py

dfc8150

MAINT: rename uniform_sampling->uniform_grid

16acab2

- Similarly uniform_sampling_fromintv->uniform_grid_fromintv - Remove old uniform_grid

REL: get version in setup.py from package file

838ca72

MAINT: minor fixes

799f2e5

kohr-h force-pushed the issue-670__rect_grid branch from 61a29db to 799f2e5 Compare January 22, 2017 18:35

kohr-h mentioned this pull request Jan 22, 2017

Order of sections in operator docstrings #854

Open

kohr-h merged commit 70919bd into odlgroup:master Jan 22, 2017

kohr-h deleted the issue-670__rect_grid branch January 22, 2017 18:41

This was referenced Jan 23, 2017

Correct weighting of RayTransform with non-uniform angles #267

Open

How to treat mixed uniform and non-uniform discretization in tomography? #286

Open

kohr-h mentioned this pull request Aug 26, 2018

Op repr updates #1394

Open

3 tasks

MAINT: rename TensorGrid->RectGrid and remove RegularGrid #841

MAINT: rename TensorGrid->RectGrid and remove RegularGrid #841

Conversation

kohr-h commented Jan 20, 2017

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

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

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 commented Jan 21, 2017 • edited Loading

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

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 commented Jan 21, 2017 •

edited

Loading