hipspy / hips

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

Add progress bar support for fetching and drawing HiPS tiles #105

Closed adl1995 closed 7 years ago

adl1995 commented 7 years ago

As mentioned in #80, I have added a progress bar support for fetching and drawing HiPS tiles. The progress bar is off by default, the result can be seen in this notebook: https://github.com/adl1995/HIPS-to-Py/blob/master/examples/Precise%20drawing-ICRS.ipynb

To achieve this, I had to make use of the tqdm module.

adl1995 commented 7 years ago

@cdeil The test fails with this error:

ModuleNotFoundError: No module named 'tqdm'

Do we have to manually specify the required packages on Travis CI?

cdeil commented 7 years ago

Do we have to manually specify the required packages on Travis CI?

Yes, you have to add a new dependency like tqdm to .travis.yml as well as to setup.py. A similar example is mypy:

.travis.yml
77:               PIP_DEPENDENCIES='mypy pycodestyle'

setup.py
114:        'mypy>=0.501',

One question I have is whether we should make this a required or optional dependency!?

I think I would prefer if it's an optional dependency, but that would mean that the implementation has to be changed to remove the import tqdm at the top of the file and instead only import it when the user wants it:

def get_progress_bar

and then use it like this:

if self.progress_bar:
    from tqmd import tqmd
    tiles = tqmd(self.draw_tiles, desc='Drawing tiles')
else:
    tiles = self.draw_tiles

# Then the same as before
for tile in tiles:
    # process the tile

I'm also worried a bit about adding complex stuff like progress bars without any tests that show that it works. But I think the complexity of testing the progress bar functionality is probably unreasonably high that we don't attempt this, but instead just test it ourselves and rely on users to report issues if there are any now or in the future as the code changes.

adl1995 commented 7 years ago

@cdeil I have made tqdm an optional dependency now, please check. I verified it on this notebook: https://github.com/adl1995/HIPS-to-Py/blob/master/examples/Precise%20drawing-ICRS.ipynb

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.6%) to 96.118% when pulling e2c9c69989072fffb6271b43273fee65d186e274 on adl1995:progress_bar into a90e5fe4b22733b6bf92a96b6d0da88fc2f2b4e6 on hipspy:master.

adl1995 commented 7 years ago

@cdeil I made the changes. The tests are passing locally with the progress bar turned on. Let's see if Travis CI build passes.

cdeil commented 7 years ago

@adl1995 - See the fails on travis-ci. Can you fix or should I explain how?

adl1995 commented 7 years ago

Okay, the test fails because it can't find the tqdm module. As this is an optional dependency, should I still update the .travis.yml file?

cdeil commented 7 years ago

As this is an optional dependency, should I still update the .travis.yml file?

I only had a very quick look, but yes, I think you need updates to .travis.yml and also to mark the tests as requires_dependency('tqdm') or to change the tests so that they run with progress_bar=False, no?

adl1995 commented 7 years ago

I added tqdm as a dependency, however, I haven't marked the tests with requires_dependency('tqdm') as I don't know where to import this marker from.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 96.429% when pulling fcb26cfc11621f43186555d74a074c30a59fd984 on adl1995:progress_bar into a90e5fe4b22733b6bf92a96b6d0da88fc2f2b4e6 on hipspy:master.

cdeil commented 7 years ago

Thanks!

cdeil commented 7 years ago

Small follow-up commit in master: 3d06423