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

Report to users what's going on #80

Open
cdeil opened this issue Jul 19, 2017 · 4 comments
Open

Report to users what's going on #80

cdeil opened this issue Jul 19, 2017 · 4 comments
Assignees
Milestone

Comments

@cdeil
Copy link
Contributor

cdeil commented Jul 19, 2017

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:

  • how many tiles / megabyte are being fetched
  • when tile fetching starts
  • when tile drawing starts
  • how long fetching and drawing took
  • how much memory was or is used

@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.
@cdeil cdeil added this to the 0.2 milestone Jul 19, 2017
@cdeil cdeil mentioned this issue Jul 19, 2017
@adl1995
Copy link
Member

adl1995 commented Jul 25, 2017

@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
Copy link
Contributor Author

cdeil commented Jul 25, 2017

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
Copy link
Contributor Author

cdeil commented Jul 26, 2017

I had a look at the astropy.utils for progress update just now. Some slightly related comments here: astropy/astropy#4738 (comment)

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
Copy link
Contributor Author

cdeil commented Jul 26, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants