Skip to content
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

Add property 'children' for creating four children tiles from parent tile #85

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

adl1995
Copy link
Member

@adl1995 adl1995 commented Jul 20, 2017

As outlined in #83, I have added a property named children in HipsTile class, this splits the parent tile into four children tiles. I also added a some test cases for the first child.

@cdeil cdeil self-assigned this Jul 20, 2017
@cdeil cdeil added this to the 0.2 milestone Jul 20, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.847% when pulling 8de1614 on adl1995:children_tiles into 438bbe4 on hipspy:master.

@cdeil
Copy link
Contributor

cdeil commented Jul 20, 2017

The splitting isn't correct. For a tile of shape (512, 512), all the children should have shape (256, 256). You need to fix the implementation.

For the tests: please make a separate test_children method, don't mix the tests for the children property in with test_read.

Finally, even if the output shape of the children is correct, I guess it'll still not be clear whether their orientation is correct, or whether they are flipped in x or y or rotated. You could make a notebook drawing once the parent tile, and then the children, and show that the resulting sky image is (almost, apart from small distortiion correction) the same. I leave it up to you if you want to do this check now, or in the next PR where you use these children for drawing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.847% when pulling 8de1614 on adl1995:children_tiles into 438bbe4 on hipspy:master.

@adl1995
Copy link
Member Author

adl1995 commented Jul 20, 2017

@cdeil I have updated the implementation. Please check if it's correct now. For the Jupyter notebook, I would prefer to create it along with the next pull request.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.854% when pulling 6a45172 on adl1995:children_tiles into 438bbe4 on hipspy:master.

@cdeil
Copy link
Contributor

cdeil commented Jul 20, 2017

The implementation now splits into 4 parts with the correct shapes. But I still don't know if it's correct. You are putting the child with data[0: w, 0: w] to correspond to meta.ipix * 4 + 0, but that is just a guess, right? The correspondence which data sub-part goes with which HEALPIX ipix for the child is unclear and non-obvious (and thus probably incorrect).

My preference would be that you debug this here, because the next pull request will be even more complex, with changes to the painter, so debugging this utils function there in addition could take very long. Whereas this here is a very small pull request, and you will manage to understand, debug, fix and then document via a comment in the code the child tile order and orientation.

It's not so hard, it should be ~ 20 lines of code and take an hour: you just make a notebook where you draw a single tile, and then draw the four sub-tiles. Either the output images are correct, or you'll see that the children are in the wrong place or flipped and then adjust your code until it's correct.

--

For the test: remove all changes to the test_read() method. And change test_children to assert something about each of the four child tiles that establishes the current behaviour. Concretely I would assert on the ipix of each child, and the pixel value at pixel index (0, 255).

@pytest.mark.parametrize('pars', HIPS_TILE_TEST_CASES)
def test_children(self, pars):
tile = self._read_tile(pars)
child1_data = tile.children[0].data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I would suggest to put this on a separate line:

child_tiles = tile.children

Then you can on the following lines access meta and data from each child as you like, e.g. you could do:

ipix = [_.meta.ipix for _ in child_tiles]

and then assert on this ipix list, to establish the current behaviour of the function about the order of child tiles returned.

@adl1995
Copy link
Member Author

adl1995 commented Jul 20, 2017

@cdeil I just tried displaying the found children tiles and compared it with the father tile, the result looks good to me. Link to notebook: https://github.com/adl1995/HIPS-to-Py/blob/master/examples/Tile%20distortion%20issue%20-%20Fix.ipynb

@cdeil
Copy link
Contributor

cdeil commented Jul 20, 2017

Now project these tiles onto the sky image, and see if they land in the right place / with the right orientation. If it's not clear to you what to do here, let's chat on Slack.

@adl1995
Copy link
Member Author

adl1995 commented Jul 20, 2017

@cdeil I updated the code according to your suggestions. The drawing code isn't updated yet, I will do that in the next PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 96.854% when pulling e08ddcd on adl1995:children_tiles into fe19120 on hipspy:master.

@cdeil cdeil modified the milestones: 0.1, 0.2 Jul 20, 2017
@cdeil
Copy link
Contributor

cdeil commented Jul 20, 2017

Mui bien!

@cdeil cdeil merged commit 354eb15 into hipspy:master Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants