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

DM-14774 Plotly upgrade #627

Merged
merged 3 commits into from
Jun 28, 2018
Merged

DM-14774 Plotly upgrade #627

merged 3 commits into from
Jun 28, 2018

Conversation

tgoldina
Copy link
Contributor

@tgoldina tgoldina commented Jun 21, 2018

https://jira.lsstcorp.org/browse/DM-14774

Deployment: https://irsawebdev9.ipac.caltech.edu/dm-14774/firefly/

  • Upgraded plotly to 1.38.3,
  • Optimized scatter to use WebGL for large tables,
  • Added an experimental option to test Plotly.react
  • Updated build to copy plotly distribution from firefly

Updated chart options (with firefly defaults):

// maximum table rows for scatter chart support, undefined means unlimited
maxRowsForScatter: undefined,
// maximum table rows for which the default chart is scatter, heatmap is created for larger tables
maxRowsForDefaultScatter: 5000,
// minimum number of points to use WebGL 'scattergl' instead of SVG 'scatter'
minScatterGLRows: 1000,

To use Plotly.react instead of our own optimizations, set in console
sessionStorage.setItem('plotlyReact', true)

To see plot render time use
firefly.debug = true

@ejoliet please review the numbers above - would you like any changes for irsa apps?

There is a separate pull request for ife.

scattergl is up to 10x faster on restyle, hence I am switching behind the scenes to scattergl for more than 1000 rows, and back when it's less than 1000 rows. (The number comes from minScatterGLRows property above.) It looks like scatter and scattergl traces now work together, so there is no issue of mixing them up. Error bars are not well supported in scattergl (they tend to disappear on relayout), so I felt that we still want to use SVG version of scatter for smaller charts.

@tgoldina tgoldina requested review from ejoliet and loitly June 21, 2018 16:11
@tgoldina tgoldina self-assigned this Jun 21, 2018
@tgoldina tgoldina changed the title Dm 14774 plotly DM-14774 Plotly upgrade Jun 21, 2018
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

Only in Safari=> AllWISE source catalog, m51, 2500+ radius. Page will not load. A problem repeatedly occurred ...

Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

This is really exciting. ScatterGL performance is really good. I especially like that it can be mixed with SVG traces in the same chart. This is a good improvement.

maxRowsForScatter: 5000, // maximum table rows for scatter chart support
maxRowsForScatter: undefined, // maximum table rows for scatter chart support, undefined means unlimited
maxRowsForDefaultScatter: 5000, // maximum table rows for which the default chart is scatter, heatmap is created for larger tables
minScatterGLRows: 1000, // minimum number of points to use WebGL 'scattergl' instead of SVG 'scatter'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still desirable to switch to heatmap on large scatter now that we can display large scatter using GL?

@ejoliet
Copy link
Contributor

ejoliet commented Jun 23, 2018

I'm trying this search in latest Firefox but chart doesn't load:
https://irsawebdev9.ipac.caltech.edu/dm-14774/irsaviewer/?__action=table.search&request=%7B%22startIdx%22%3A0%2C%22pageSize%22%3A100%2C%22use%22%3A%22catalog_overlay%22%2C%22SearchMethod%22%3A%22Cone%22%2C%22RequestedDataSet%22%3A%22AllWISE%20Database%22%2C%22id%22%3A%22GatorQuery%22%2C%22tbl_id%22%3A%22tbl_id-c0-4%22%2C%22META_INFO%22%3A%7B%22title%22%3A%22WISE-allwise_p3as_psd%20(Cone%3A1000%27%27)%22%2C%22tbl_id%22%3A%22tbl_id-c0-4%22%2C%22DataTag%22%3A%22catalog%22%2C%22backgroundable%22%3Atrue%7D%2C%22catalogProject%22%3A%22WISE%22%2C%22catalog%22%3A%22allwise_p3as_psd%22%2C%22UserTargetWorldPt%22%3A%2210.68479%3B41.26906%3BEQ_J2000%3Bm31%3Bned%22%2C%22radius%22%3A1000%2C%22constraints%22%3A%22%22%7D&options=%7B%22backgroundable%22%3Atrue%2C%22pageSize%22%3A100%7D

@xiuqin
Copy link
Contributor

xiuqin commented Jun 28, 2018

Running in Chrome Version 67.0.3396.62 (Official Build) (64-bit)
plotting 23,799 points took less than 2 seconds, while the old one took about 10 seconds.
This is a great performance improvement!

@xiuqin
Copy link
Contributor

xiuqin commented Jun 28, 2018

I also tried on FireFox Quantum (60.0.2 (64-bit), the same search and scatter plot without any issue.
@ejoliet Could it be merged to dev?

@tgoldina tgoldina merged commit b3069a3 into dev Jun 28, 2018
@tgoldina tgoldina deleted the dm-14774_plotly branch June 28, 2018 18:38
@tgoldina
Copy link
Contributor Author

tgoldina commented Jun 28, 2018

I have updated IrsaViewer.js and irsaviewer/ts.html to make sure the chart behavior stays as before: no WebGL for now. In Firefly, we'll use WebGL for all scatters with more than 1000 points to make sure WebGL gets tested.

@loitly
Copy link
Contributor

loitly commented Jun 28, 2018

I have updated IrsaViewer.js and irsaviewer/ts.html to make sure the chart behavior stays as before: no

@tgoldina , shouldn't it be irsaviewer.html and app.html in ./ife/src/irsa as well? finderchart is using app.html and irsaviewer.html is also used in ops.

@tgoldina
Copy link
Contributor Author

There are no chart options in app.html and irsaviewer.html. I assume they get picked up from IrsaViewer.js Please correct me if I am wrong.

@loitly
Copy link
Contributor

loitly commented Jun 28, 2018

@tgoldina , so yes irsaviewer.html and ts.html uses IrsaViewer.js. However, app.html uses IrsaEntry.js. Just wanted to remind you all of the involved files. I don't know if they need to be modify or not.

@tgoldina
Copy link
Contributor Author

Thank you. Updated IrsaEntry.js too.

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.

4 participants