hipspy / hips

Python library to handle HiPS
https://hips.readthedocs.io
12 stars 16 forks source link

Reformat SimpleTilePainter class #66

Closed adl1995 closed 7 years ago

adl1995 commented 7 years ago

As discussed in issue #57, I have reformatted the SimpleTilePainter class. I'm aware this still needs some changes, but this will provide us with a working ground. For now, I have commented out the test_draw_sky_image function as it is not currently usable.

@cdeil @tboch Please provide feedback on what needs to be changed.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.8%) to 96.825% when pulling 22b1bb6c6bb73ff2e7a0b75980dd46ab4d730f0e on adl1995:refactor_simple into e29054cd6c32ff4c61160c198de504dfb06fca23 on hipspy:master.

adl1995 commented 7 years ago

@cdeil I made some changes. Although, I'm not sure if the make_sky_image function is required now, as it basically just calls SimpleTilePainter.run(). Also, I couldn't add tests for png tiles because they are not available in hips-extra (the ones that fall on the galactic center).

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.082% when pulling ca8b6902094b76716fb329554d4297ebe832a100 on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

cdeil commented 7 years ago

I'll have a look now. My suggestion would be to keep the "make_sky_image" function, as the very high level, very simple end user interface. Once we have different tile fetching / caching and drawing methods, the code in make_sky_image would again grow to 10 or 20 lines of configuring / calling the lower-level routines via a uniform and simple to configure interface (e.g. a draw_method="simple" or draw_method="fancy" method.

make_sky_image should also return a result object that offers more info / data members than just the numpy array of pixels we currently return.

Makes sense?

cdeil commented 7 years ago

Also, I couldn't add tests for png tiles because they are not available in hips-extra (the ones that fall on the galactic center).

Can you make a PR adding the tiles you need yourself? If yes, please do. If no, please file an issue and I or Thomas will do it.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.082% when pulling 7f3efdf2889a57a93652a892b6d800d5039289d3 on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.082% when pulling becda2940f96f24f52c72fe66df0e3290d77428c on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

adl1995 commented 7 years ago

@cdeil I just made some changes.

cdeil commented 7 years ago

Please always run python setup.py build_docs locally and resolve all warnings / errors: https://travis-ci.org/hipspy/hips/jobs/253249006#L1158

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.12% when pulling 2840a12f8bc04321dfb0ec5591febde5c8de7917 on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

cdeil commented 7 years ago

@adl1995 - I left some more small comments inline. Once those are adressed, and there's green light on travis ci (still the docs build: https://travis-ci.org/hipspy/hips/jobs/253258805#L1158), I'll merge.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.12% when pulling 201fc428b783a7f74cb6ca51e3ca81c297454955 on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

cdeil commented 7 years ago

@adl1995 - I wanted to start playing around with HiPS tile drawing a bit myself: https://github.com/cdeil/hipsperiments/blob/master/rgb_tile_draw_issue.ipynb

The problem with the current class is that one cannot access the tiles any more after running the algorithm.

Can you please change the implementation for painter.tiles to return a list of tiles, instead of a being a generator.

Basically I would propose this coding pattern:

class Painter:
    def __init__(self):
        self._tiles = None

    @property
    def tiles(self):
        if self._tiles is None:
            self._tiles = list(self._fetch_tiles())  # the method you have now, just with a leading underscore
        return self._tiles

This way, the painter fetches the tiles only once, and puts them in his pocket in case someone (like me) wants to inspect them later.

@adl1995 - is it clear? do you want to implement it or should I just take over here?

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.2%) to 97.409% when pulling eb5cd378624001de9588a4eb7095361b1f9b94d5 on adl1995:refactor_simple into 1f0ef5b2753e1b58c6b7d88f028ec8a2ad666104 on hipspy:master.

adl1995 commented 7 years ago

@cdeil I made some updates to the code and also added a test case for PNG tiles. The Travis CI is passing now. Please check.

cdeil commented 7 years ago

@adl1995 - Thanks!

I'm merging this now.

PS: I'll do some cleanup and coding on hips in the next ~ 2 hours, to polish a bit for the upcoming v0.1 release.