Add method to_split in SimpleTilePainter for checking whether to split a tile #89

adl1995 · 2017-07-24T09:13:45Z

As mentioned in issue #84, I have added two methods to_split and _measure_tile_shape which are used for checking whether a tile needs to be split or not. The next step is to perform the actual splitting and modify the projection part to take in the correct tile_width (half, in case a tile is split).

cdeil

Please add a unit test for _measure_tile_shape, establishing the results you get for one case (assert on all the return values).

For test_to_split, it would be good to have a second test case of a tile that isn't split, no?

For the implementation, I think you need to handle the case where diagonals[0] > diagonals[1] and also diagonals[1] < diagonals[0]. One way to do it would be to compute both numbers and then take the min. Another way could be to only define one ratio by directly making sure it's computed so that ratio < 0. Overall I think that ratio computation could also happen in to_split.

You are duplicating the code that computes the cartesian distance (the formula with np.sqrt four times). I think this is easily avoided by defining a helper function def compute_distance(i, j) as a nested function inside _measure_tile_shape that computes the distance between points i and j, and to call that function a few times, no?

Finally, I think the function that decides whether to split can just take the corners array, i.e. I would suggest to move the line corners = meta.skycoord_corners.to_pixel(wcs) to the caller, to be executed before calling is_tile_distorted (that's the name I would suggest, it's always good to name things that return bools in such a way that it's clear that it's a bool.

coveralls · 2017-07-24T09:39:05Z

Coverage increased (+0.1%) to 96.792% when pulling 71be503 on adl1995:tile_split into fce110c on hipspy:master.

adl1995 · 2017-07-24T10:38:01Z

@cdeil I just made the updates.

cdeil

I left some more inline comments, mainly suggesting to simplify the implementation of _measure_tile_shape.

cdeil · 2017-07-24T10:44:57Z

hips/draw/simple.py

+
+        return [edges, diagonals, ratio]
+
+    def is_tile_distorted(self, corners: tuple) -> bool:


Make this a @staticmethod

cdeil · 2017-07-24T10:46:58Z

hips/draw/tests/test_simple.py

@@ -101,3 +102,21 @@ def test_draw_debug_image(self):
        tile = self.painter.tiles[3]
        image = self.painter.image
        plot_mpl_single_tile(self.geometry, tile, image)
+
+    def test_to_split(self):


Rename to test_ is_tile_distorted.

cdeil · 2017-07-24T10:52:31Z

hips/draw/simple.py

+    def _measure_tile_shape(corners: tuple) -> List[list]:
+        """Compute tile edges and diagonals."""
+
+        def compute_distance(i: list, j: list) -> float:


I find this way you've implemented compute_distance hard to read.
My suggestion would be that you move the

x, y = corners

to the top and then do this:

def compute_distance(i, j): return np.sqrt( (x[i] - x[j]) ** 2 + (y[i] - y[j]) ** 2)

i.e. have i and j as the integer index of the corner.

Then you should be able to call it like this:

edge_lengths = [compute_distance(i, (i+1) %4) for i in range(4)]

and like this

diagonal_lengths = [compute_distance(0, 2), compute_distance(1, 3)]

Note that I didn't try this code, I just typed it here.
Can you please try to change the implementation to work this way?

cdeil · 2017-07-24T10:53:32Z

hips/draw/simple.py

+        """Implement tile splitting criteria as mentioned in :ref:`drawing_algo` page."""
+        edges, diagonals, ratio = self._measure_tile_shape(corners)
+
+        return any(i > 300 for i in edges) or \


I think I would find max(edge_lengths) > 300 and equivalently for diagonals easier to read.

coveralls · 2017-07-24T11:02:15Z

Coverage increased (+0.1%) to 96.798% when pulling cdb2003 on adl1995:tile_split into fce110c on hipspy:master.

adl1995 · 2017-07-24T14:20:36Z

@cdeil Okay, I made the changes. However, the test results vary from before. I'm not sure if these are the correct ones or if the previous ones were.

coveralls · 2017-07-24T14:46:22Z

Coverage increased (+0.08%) to 96.768% when pulling 1d98898 on adl1995:tile_split into fce110c on hipspy:master.

coveralls · 2017-07-24T15:14:55Z

Coverage increased (+0.08%) to 96.768% when pulling 1d98898 on adl1995:tile_split into fce110c on hipspy:master.

cdeil

I left some more inline comments.

However, the test results vary from before. I'm not sure if these are the correct ones or if the previous ones were.

My guess would be that there was a bug before and that now the results are correct, because the implementation looks good to me.

One thing that would help to make sure it's correct would be if the corner x / y coordinates were visible in the test, i.e. if you'd add an assert_allclose to make them "visible" and "checkable just by reading the test code". If I see the X / Y coordinates, I can compute by hand what the distances are (basically by duplicating the code you're implementing here though). For me, that would be enough.

If you really want to be sure, and prove to yourself that the implementation is correct now, you could also make a Jupyter notebook where you draw the tile, and then draw circles around the corners with the computed lengths, and see of they go through the corresponding other points, i.e. have the right length. (From my side, this is not necessary, I believe that the implementation you have now is OK, apart from the bug that you have %3 where it should be %4 that I implemented out as an inline comment).

cdeil · 2017-07-24T19:38:55Z

hips/draw/simple.py

+            """Compute distance between two points."""
+            return np.sqrt((x[i] - x[j]) ** 2 + (y[i] - y[j]) ** 2)
+
+        edges = [compute_distance((i + 1) % 3, i) for i in range(4)]


You now have %3. Shouldn't this be %4 instead?

cdeil · 2017-07-24T19:39:28Z

hips/draw/simple.py

@@ -117,6 +117,30 @@ def _fetch_tiles(self) -> HipsTile:
            tile = HipsTile.fetch(tile_meta, url)
            yield tile

+    @staticmethod
+    def _measure_tile_shape(corners: tuple) -> List[list]:
+        """Compute tile edges and diagonals."""


"Compute tile edges and diagonals" -> "Compute length of tile edges and diagonals"

cdeil · 2017-07-24T19:40:39Z

hips/draw/simple.py

+        return [edges, diagonals, ratio]
+
+    @staticmethod
+    def _is_tile_distorted(self, corners: tuple) -> bool:


Remove self argument here, it's never passed to @staticmethods.
You then have to call SimpleTilePainter._measure_tile_shape(corners) instead of self._measure_tile_shape(corners).
Overall my suggestion here would be to move these two functions just out of the SimpleTilePainter class, and make them standalone functions and put them above or below the SimpleTilePainter class.

…t a tile

adl1995 · 2017-07-25T05:15:24Z

@cdeil Alright, just made the changes. Please check.

coveralls · 2017-07-25T05:37:48Z

Coverage increased (+0.07%) to 96.756% when pulling 808e758 on adl1995:tile_split into fce110c on hipspy:master.

cdeil

Looks good now. Thanks!

cdeil · 2017-07-25T06:46:01Z

@adl1995 - FYI: I've done a small update to this code in 1b36234
The small advantage of having the test this way is that it's independent and faster.

Another thing I noticed was that I had to put bool explicitly for is_tile_distorted. Previously it was returning numpy.bool_ which has the very surprising behaviour:

>>> np.bool_(True) is True
False

cdeil self-assigned this Jul 24, 2017

cdeil added the enhancement label Jul 24, 2017

cdeil added this to the 0.1 milestone Jul 24, 2017

cdeil suggested changes Jul 24, 2017

View reviewed changes

adl1995 force-pushed the tile_split branch from 71be503 to cdb2003 Compare July 24, 2017 10:37

cdeil suggested changes Jul 24, 2017

View reviewed changes

adl1995 force-pushed the tile_split branch 2 times, most recently from bee6ebe to 1d98898 Compare July 24, 2017 14:19

cdeil suggested changes Jul 24, 2017

View reviewed changes

Add method to_split in SimpleTilePainter for checking whether to spli…

808e758

…t a tile

adl1995 force-pushed the tile_split branch from 1d98898 to 808e758 Compare July 25, 2017 05:13

cdeil approved these changes Jul 25, 2017

View reviewed changes

cdeil merged commit baba027 into hipspy:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to_split in SimpleTilePainter for checking whether to split a tile #89

Add method to_split in SimpleTilePainter for checking whether to split a tile #89

adl1995 commented Jul 24, 2017

cdeil left a comment

coveralls commented Jul 24, 2017

adl1995 commented Jul 24, 2017

cdeil left a comment

cdeil Jul 24, 2017

cdeil Jul 24, 2017

cdeil Jul 24, 2017

cdeil Jul 24, 2017

coveralls commented Jul 24, 2017

adl1995 commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

cdeil left a comment

cdeil Jul 24, 2017

cdeil Jul 24, 2017

cdeil Jul 24, 2017

adl1995 commented Jul 25, 2017 •

edited

Loading

coveralls commented Jul 25, 2017

cdeil left a comment

cdeil commented Jul 25, 2017


		return [edges, diagonals, ratio]

		def is_tile_distorted(self, corners: tuple) -> bool:

Add method to_split in SimpleTilePainter for checking whether to split a tile #89

Add method to_split in SimpleTilePainter for checking whether to split a tile #89

Conversation

adl1995 commented Jul 24, 2017

cdeil left a comment

Choose a reason for hiding this comment

coveralls commented Jul 24, 2017

adl1995 commented Jul 24, 2017

cdeil left a comment

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

coveralls commented Jul 24, 2017

adl1995 commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

cdeil left a comment

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

cdeil Jul 24, 2017

Choose a reason for hiding this comment

adl1995 commented Jul 25, 2017 • edited Loading

coveralls commented Jul 25, 2017

cdeil left a comment

Choose a reason for hiding this comment

cdeil commented Jul 25, 2017

adl1995 commented Jul 25, 2017 •

edited

Loading