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

added option to download an image of the most recent plot #494

Merged
merged 40 commits into from
Jun 15, 2016

Conversation

yankev
Copy link
Contributor

@yankev yankev commented Jun 2, 2016

  • This change is for the offline notebook mode
  • This is the first attempt at it, there may be better solutions that I plan to look at
  • Need to add docstring

@yankev
Copy link
Contributor Author

yankev commented Jun 3, 2016

Made the inline download method a part of the iplot call.
You can initiate it by setting the parameter download_image to True, and providing details about the format, height, width and filename.
Issue right now is that the plots will download each time the notebook is restarted using this method.

Note: This can be applied to other methods later on, could also put the script injection into another function similar to get_plotlyjs

The previous method allowed you to download the plot that was most recently plotted and this wouldn't run when the notebook was rerun.

@yankev
Copy link
Contributor Author

yankev commented Jun 6, 2016

Demo of the functionality (for Issue #483):

Right now have I have two ways of downloading the plot:

The first way involves adding a few keyword arguments to existing plot and iplot functions, indicating whether or not an image should be downloaded.

  • Inline Methods (offline.plot)
    plot_download
  • Inline Methods (offline.iplot)
    iplot_download
  • The second method will download the image of the most recent plot displayed in your ipython notebook. This will be a new function, and no other existing code/methods will be changed, compared to the previous method.

(offline.download_notebook_image)
notebook_image_download

@jackparmer @cldougl @chriddyp
Which do you think would be the best method for this functionality? Also let me know of any issues/suggestions you guys have regarding these implementations.

@yankev
Copy link
Contributor Author

yankev commented Jun 7, 2016

You can also find some notebooks to test the functionality here:
https://github.com/yankev/testing/tree/master/offline_download_image

Note: This will only wok in Firefox, the fix for Chrome will come when this fix get's released.

@jackparmer
Copy link
Contributor

Hey @yankev -
Nice! This looks cool!
I like the explicitness of the first option. Its a single step to get an image for a plot. And I don't have to remember the name of a new function.
I'd also suggest:

  • Change download_image kwarg to image
  • Don't necessarily have to include a filetype (defaults to PNG) or filename (defaults to plot_image.xyz)
  • Once this works, add a Save images offline section here https://plot.ly/python/static-image-export/ and comb back through the StackOverflow and community.plot.ly Q's about this

@jackparmer
Copy link
Contributor

We may want to print a message also in Jupyter notebook:

For higher resolution images and more export options, try Plotly's online image export server.

I'm worried we're going to get a lot of complaints that the client-side image export is not sufficiently fully featured, because folks don't realize we have also have a full-blown server just for image export.

@yankev
Copy link
Contributor Author

yankev commented Jun 11, 2016

@theengineear
Changes:
I moved the warnings into the confirm prompt. Also created a function to return the script strings. Also using the single image parameter vs both image and format.

Addressing some concerns:
I think the one second sleep time and now the confirmation box should allow for enough time for the plot to render on screen, and thus downloadImage shouldn't return a blank image. I've tested on some fairly "complicated" plots, and it seems to work. Though there is no guarantee on this one.

I have the condition to check whether the page is loaded before injecting the download script, so on page reloads/re-opening the notebook, the confirm and download prompts shouldn't show up.

@theengineear
Copy link
Contributor

I have the condition to check whether the page is loaded before injecting the download script, so on page reloads/re-opening the notebook, the confirm and download prompts shouldn't show up.

Sweet!

Taking a peek right now.

check_end = '}}'
elif type == 'plot':
check_start = ''
check_end = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄

else:
    raise SomeException( .. )

?

@theengineear
Copy link
Contributor

Great! Let's do it! 💃 after you've considered my comments.

@yankev
Copy link
Contributor Author

yankev commented Jun 14, 2016

@theengineear k, should be good. I put in a neat check to find the the name of the caller of get_image_download_script (reference: http://stackoverflow.com/a/900413/5557105).

@@ -48,14 +49,26 @@ def get_plotlyjs():
plotlyjs = resource_string('plotly', path).decode('utf-8')
return plotlyjs

def image_download_script(type):
def get_image_download_script(caller):
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 We typically use """, not ''' for docstrings.

@theengineear
Copy link
Contributor

@yankev sorry to be a party 💩-er, but I think you should stick with the more straight-forward implementation for inferring who called the function.

If it makes you feel better, you could use no_reload_confirm=True or something instead of iplot vs plot. I think your caller='iplot' implementation was just fine though 😉

@yankev
Copy link
Contributor Author

yankev commented Jun 15, 2016

@theengineear k seems to be in working shape. Let me know if there's anything else.

@theengineear
Copy link
Contributor

Beep, boop, 💃. Nice work!

@yankev
Copy link
Contributor Author

yankev commented Jun 15, 2016

@theengineear thanks man! This was a super messy one, so many avoidable errors. Thanks for reviewing it!

@yankev yankev merged commit 079c924 into master Jun 15, 2016
@cldougl cldougl deleted the download-image-offline branch March 24, 2017 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants