hipspy / hips

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

Report to users what's going on #80

Open cdeil opened 7 years ago

cdeil commented 7 years ago

The current user experience with our package is pretty bad.

If I call make_sky_image to make a large sky image: https://gist.github.com/cdeil/796875efe4a3620987b9f62eb278cfbc it takes ~ a minute and I have no idea what's going on.

We should change our code to (optionally, at medium verbosity level by default) communicate to users what's happening:

@adl1995 - I'm not sure how best to implement this yet. I guess we could start emitting log messages, or we could use a progress bar (http://docs.astropy.org/en/stable/api/astropy.utils.console.ProgressBarOrSpinner.html) ?


One thing I definitely think we should implement is a HiPSDrawResult class (or something like that), that is returned by make_sky_image instead of just the Numpy array. It would have a repr that prints the info mentioned above (how long things took, how much memory was used, how much data was fetched from the web), and also has convenience methods to write and plot the resulting sky image:

result = make_sky_image(...)
result.report() # prints to console
result.show_image() # shows the image with matplotli
result.write_image('my_image.fits')` # Makes the HDU and writes it.
adl1995 commented 7 years ago

@cdeil I'm not entirely sure on how to go about implementing the HipsDrawResult class. One thing I thought of is to introduce new methods in SimpleTilePainter for memory usage and data fetched and then call these from HipsDrawResult?

But, that doesn't seem like the best solution. I think the best way would be to contain all of the functionality within HipsDrawResult class, however, how would I get all the information like how much data is fetched, memory consumed, etc.?

cdeil commented 7 years ago

Yes, eventually changes to the SimpleTilePainter will be needed, e.g. to compute these things. But to start with, for the first pull request, please keep the changes to SimpleTilePainter to a minimum.

My suggestion would be that you introduce the new HipsDrawResult class, and change the

return painter.image

in make_sky_image to

return painter.result

and then implement a result property on the SimpleTilePainter that creates and returns a HipsDrawResult instance, based on already available info, starting with image and geometry.

Of course one could also return the painter to give the user access to all the results, that would be a simple and OK solution. The idea of introducing a new results class is to a certain degree to give users something simple with just the most common things they want, and to not have to find the "useful bits" on the painter, besides the complex painting algorithm-related things they won't care about.

cdeil commented 7 years ago

I had a look at the astropy.utils for progress update just now. Some slightly related comments here: https://github.com/astropy/astropy/issues/4738#issuecomment-317981080

Bottom line for the hips package: I think using astropy.utils.ProgressBar to display progress to users on tile fetching and maybe also drawing is probably the way to go. But it should be possible to silence this via a config option in the high-level API (called "verbose" or "verbosity" or "loglevel" or "progress"), and we have to investigate how it would interact with us emitting log messages, and see if it can be made to give a useful progress update info in the Jupyter notebook.

cdeil commented 7 years ago

Actually I had a look at tqdm and changed my mind, I think we should use that directly and not astropy.utils.ProgressBar. It is a light dependency (pure Python, no dependencies itself), is widely available, well maintained, well documented, and looks very nice on the console and in the Jupyter notebook: https://github.com/tqdm/tqdm#ipython-jupyter-integration and it can give progress like on the console in the Jupyter output even if the notebook extension isn't enabled.