Feature/plotly #410

SridharJagannathan · 2019-08-17T16:11:05Z

Added support for plotly plotting engine.
Added test cases for the same.
Items that need to be checked:

I have done manual testing (comparing with outputs of rgl) of most of the features, however it would be nicer for a more experienced person to check the details of a well known neuron or neuron list or surface etc.
Have not implemented plot3d.character as it returns an error by default even for rgl implementation.

Would close #409

coveralls · 2019-08-17T16:22:51Z

Coverage decreased (-0.09%) to 83.313% when pulling 04ff657 on feature/plotly into 3ab1eda on master.

jefferis · 2019-08-17T17:42:13Z

Thanks @SridharJagannathan! I think that you should not need to make any changes to plot3d.character - it should work transparently if you specify a plotengine argument.

jefferis · 2019-08-17T18:03:48Z

I think it would be good to have an option nat.plotengine that defines whether rgl or plotly is the global default.

* master: Fix bug in nmapply with progress fix warning in teardown skip exact md5 ndigest tests on CRAN nat.examples is still on jefferis user redown inc with logos Give neuronlistfh/fillMissing new progress bar Give nmapply a progress bar Export progress_natprogress for other packages Fix URL typo in README Teach cmtk.bindir about compiled but uninstalled cmtk Set default filename for write.neurons from neuronlist name Add progress bar for write.neurons Make plot.cmtkreg example conditional docs / examples for new progress bar Make nlapply use progress package reporter by default give read.neurons a progress bar with ETA

jefferis · 2019-08-27T10:29:29Z

This is looking very good. I was thinking that one advantage was that it might be possible to convert the examples in the pkgdown docs to use the plotly engine so that the 3D content would appear. But two issues

there is no exported equivalent of clear3d() and it would have been better if it were the same regardless of plot engine.

a bug as follows in in some example code:

> plot3d(kcs20)
> plot3d(nlapply(kcs20,boundingbox))
 Error in UseMethod("layout") : 
no applicable method for 'layout' applied to an object of class "NULL"

> traceback()
11: plotly::layout(., showlegend = FALSE, scene = list(camera = .plotly3d$camera))
10: function_list[[k]](value)
9: withVisible(function_list[[k]](value))
8: freduce(value, `_function_list`)
7: `_fseq`(`_lhs`)
6: eval(quote(`_fseq`(`_lhs`)), env, env)
5: eval(quote(`_fseq`(`_lhs`)), env, env)
4: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
3: plotlyreturnlist$plotlyscenehandle %>% plotly::layout(showlegend = FALSE, 
       scene = list(camera = .plotly3d$camera)) at neuronlist.R#727
2: plot3d.neuronlist(nlapply(kcs20, boundingbox))
1: plot3d(nlapply(kcs20, boundingbox))

jefferis · 2019-08-27T10:34:39Z

Another small point, I think plotly_plotonce could be merged into skipRedraw since they are about the same kind of thing. One could make the default dependent on the plotengine setting.

jefferis · 2019-08-27T10:35:36Z

PS @SridharJagannathan I am taking a look at this branch at the moment and you are enjoying a break so these notes are for info, not for you to act on now.

jefferis · 2019-09-06T17:37:00Z

A couple more points

you are currently returning a plotlyreturnlist from each object which contains a $plotlyscenehandle field. You are also in some cases explicitly calling: print(plotlyreturnlist$plotlyscenehandle). However this approach does not work for rmarkdown reports. Instead I think you need to try and adhere more closely to Plotly's style. This means that your plot functions should return the scene handle visibly. In an interactive session or an rmarkdown document, the return value of the top level function will be dynamically printed resulting in the display of the plot. This actually handles transparently some of the issues that you run into about when to show the plot and that you try to control with plotly_plotonce. In particular if you do this, you don't need to worry about suppressing plots in e.g. plot3d.neuron when you are plotting lots of things from plot3d.neuronlist.
the issue of when to clear the Plotly scene and start afresh is a bit more complicated. Some rgl functions have add=TRUE or add=FALSE arguments that are a step in that direction but normally the assumption is that objects are added to the scene, which is explicitly cleared. We need to think more carefully about how to handle this for Plotly. I think that we will need to give a whole bunch of our plot3d.* methods the add argument with a default of TRUE. Then we could start a new scene either when one does not exist or when add=FALSE. This should remove the requirement for plotly_plotonce altogether.

jefferis · 2019-09-06T17:43:58Z

R/cmtkreg.R

+                                                         scene=list(camera=.plotly3d$camera))
+    assign("plotlyscenehandle", plotlyreturnlist$plotlyscenehandle, envir=.plotly3d)
+    print(.plotly3d$plotlyscenehandle)
+    invisible(plotlyreturnlist)


This should be returning:

.plotly3d$plotlyscenehandle

without the print statement

jefferis · 2019-09-06T17:45:28Z

R/cmtkreg.R

@@ -154,5 +165,20 @@ plot3d.cmtkreg <- function(x, ...) {
  coeffs=reg$spline_warp$coefficients
  aidxs=seq.int(from=1, by=3L, length.out = nrow(coeffs))
  actives=reg$spline_warp$active[aidxs]!=0
-  plot3d(coeffs[actives, ], xlab='X', ylab = "Y", zlab = "Z", ...)
+  if (plotengine == 'rgl'){


I think it should be

if (plotengine == 'plotly'){

and then do Plotly stuff with the rgl engine as default. I'm not sure we actually set options(plotengine='rgl') anywhere do we?

OK, you do set options(plotengine='rgl') in zzz.R so I guess this is ok.

* see #410 (comment) * add a plotly vignette to demonstrate this is actually the case

* also reduce default line width to 1 for plotly

* this is something rather nice that we get with plotly

* no longer necessary * also standardise docs for plotengine

* otherwise we could end up with a bad value and some stuff that checks for rgl not happening

* note that hover colour does not match soma unfortunately.

* add to index

jefferis · 2019-09-11T09:44:31Z

Note that I was not aware, but somewhat disappointingly it seems that pkgdown cannot yet support the use of htmlwidgets and therefore plotly in examples (vignettes are fine). See e.g. r-lib/pkgdown#742

SridharJagannathan · 2019-09-13T11:05:40Z

Just updating some comments for my notes here:

507531f, a2b1d62, 76a673a, 5270caf - ok
0a2bed7 - seems ok, but why in some places the argument plotengine is removed in the parameter description section, for e.g. look at function - plot3d.neuron has argument plotengine but its description is removed
e7879cf - seems ok, but may be in the function nopen3d we should check if the plotengine argument, and raise an error if this was called with the plotly engine.
686dadf - seems ok, I need to check what is the use of nlscan though, however I noticed that for e.g. in the example code, plot3d(FCWB, plotengine = 'rgl') produces a much better that can be adjusted for its size producing a much better mesh for visualisation however with plot3d(FCWB, plotengine = 'plotly') produces a mesh that can be adjusted and looks a bit less pleasing, hence we should consider autosizing the width for these cases.
db4802d - seems ok, but i find no test cases that actually check the workings of check_plotengine across all the plot3d functions.
8f6ca68 - the vignette looks nice and informative. I was wondering won't it be better if we time the runtime required for display by both rgl and plotly backend in the form of a table so the users can make a better choice?
5bde8e8 - seems ok, but can you provide an example or point me to one for trace names?
7233706 - ok
136c808 - ok
Also, need to check how legends work with plotly, so far i have ignored them.

SridharJagannathan · 2019-09-13T14:54:53Z

Note that I was not aware, but somewhat disappointingly it seems that pkgdown cannot yet support the use of htmlwidgets and therefore plotly in examples (vignettes are fine). See e.g. r-lib/pkgdown#742

What examples are we talking about here, can you give me a mwe?

jefferis · 2019-09-13T15:30:16Z

0a2bed7 - seems ok, but why in some places the argument plotengine is removed in the parameter description section, for e.g. look at function - plot3d.neuron has argument plotengine but its description is removed

There is now an @inheritParams statement. In this way you write the help for one function and then reuse it in other functions. This also means you only have to fix it once if there is a typo (as there was) or some additional information would be helpful.

e7879cf - seems ok, but may be in the function nopen3d we should check if the plotengine argument, and raise an error if this was called with the plotly engine.

I don't think so. The goal is that, where possible, it would be better if the same code could produce reasonable outcomes with both plot engines. So I don't think we want to throw an error here. However one change worth considering is that we make nopen3d and nclear3d do the same thing (clearing our .plotly environment) when plotengine="rgl".

686dadf - seems ok, I need to check what is the use of nlscan

It is to plot a large number of neurons, quickly, one after another and optionally select some of them. It does not really make so much senses for plotly, because you cannot currently edit a plotly scene, you just rebuild it from scratch. In contrast with plotly you may be able to plot all the neurons at once and hover to identify.

though, however I noticed that for e.g. in the example code, plot3d(FCWB, plotengine = 'rgl') produces a much better that can be adjusted for its size producing a much better mesh for visualisation however with plot3d(FCWB, plotengine = 'plotly') produces a mesh that can be adjusted and looks a bit less pleasing, hence we should consider autosizing the width for these cases.

Do you want to give an eg? I did change the line widths for your bounding box plots as they seemed very wide. I'm not sure about surface plots.

This also reminds me, that it might be worth making the default for surface hover to be no hover information when only the hxsurf object contains only one region.

db4802d - seems ok, but i find no test cases that actually check the workings of check_plotengine across all the plot3d functions.

it is exercised indirectly by every single call to the plot3d functions. But it's ability to respond to a bad value of the option is never tested explicitly, if you want to check that as well.

8f6ca68 - the vignette looks nice and informative. I was wondering won't it be better if we time the runtime required for display by both rgl and plotly backend in the form of a table so the users can make a better choice?

You could try that for sure. I'm not sure that the time to build the scene is the major determinant for webGL vs plotly choice. Building the scene with rgl is much faster, but I'm not sure that we can really time that in a markdown document.

5bde8e8 - seems ok, but can you provide an example or point me to one for trace names?

plot3d(kcs20, soma=TRUE)

try hovering over the somata – colour always stays red.

Also, need to check how legends work with plotly, so far i have ignored them.

I thought that could be interesting. I think Philipp has used them to label plots and show (groups of) neurons, then this could be useful. I think we could still finish this PR and add this as a new issue.

jefferis · 2019-09-13T15:46:51Z

Note that I was not aware, but somewhat disappointingly it seems that pkgdown cannot yet support the use of htmlwidgets and therefore plotly in examples (vignettes are fine). See e.g. r-lib/pkgdown#742

What examples are we talking about here, can you give me a mwe?

This is for the examples at the bottom of the documentation for each function on the pkgdown website for nat.

SridharJagannathan · 2019-09-13T16:13:18Z

@jefferis Thanks so much for the detailed answers, def helpful.. :)

Do you want to give an eg? I did change the line widths for your bounding box plots as they seemed very wide. I'm not sure about surface plots.

The following should reproduce the issue:

library(nat.flybrains)
plot3d(FCWB, plotengine = 'rgl')
plot3d(FCWB, plotengine = 'plotly')

If you compare the outputs of the rgl and plotly you can see that the plotly simply doesn't show the brains properly. you can see that the x-axis has the range from 0 to 550 but the other axes are smaller yet the plotly shows them as a bit larger, I have fixed that with 04ff657.

I would try the other things you mentioned and perhaps we can close this feature then.

jefferis · 2019-09-14T10:20:07Z

Hi @SridharJagannathan, I see the problem you point out with the FCWB plots above, but I think the problem is both slightly simpler and more general than your solution would imply. As far as I can see, the issue is that the aspect ratio of the axes needs to be set to one. Something like this:

aspectratio = list(x=1, y=1, z=1)

see https://plot.ly/r/3d-axes. I've attached a picture of FCWB using the old code for reference to show the squashed axes:

as well as the new code with a different scene:

which shows that extending the axes at least as you have done isn't going to work.

I think that setting the fixed axis ratio needs to happen at the start of each plotly scene i.e. nclear3d/nopen3d or similar. I would avoid doing it in every single plot command.

SridharJagannathan · 2019-09-19T14:00:42Z

Note that I was not aware, but somewhat disappointingly it seems that pkgdown cannot yet support the use of htmlwidgets and therefore plotly in examples (vignettes are fine). See e.g. r-lib/pkgdown#742

What examples are we talking about here, can you give me a mwe?

This is for the examples at the bottom of the documentation for each function on the pkgdown website for nat.

It totally see this now, so may be we wait a bit till its merged (r-lib/pkgdown#742)

SridharJagannathan · 2019-09-19T14:05:24Z

Hi @SridharJagannathan, I see the problem you point out with the FCWB plots above, but I think the problem is both slightly simpler and more general than your solution would imply. As far as I can see, the issue is that the aspect ratio of the axes needs to be set to one. Something like this:
aspectratio = list(x=1, y=1, z=1)
see https://plot.ly/r/3d-axes. I've attached a picture of FCWB using the old code for reference to show the squashed axes:

as well as the new code with a different scene:

which shows that extending the axes at least as you have done isn't going to work.

I think that setting the fixed axis ratio needs to happen at the start of each plotly scene i.e. nclear3d/nopen3d or similar. I would avoid doing it in every single plot command.

hmm, @jefferis I did try this aspect ratio, however it doesn't seem to work, see below.

jefferis · 2019-09-24T17:52:13Z

Just a note that the key here was to use

plotly::layout(scene=list(aspectmode='data'))

SridharJagannathan added 10 commits August 15, 2019 12:02

First pass for plot3d.neuron using plotly engine.

be0047e

First pass implementation for neuronlist and code clean up.

7fe0b3f

First pass implementation of plot3d.boundingbox with 'plotly' engine.

99ef4a1

Updated plot3d.dotprops for plotly engine.

599357f

Updated for plotly openplotlyscene local function.

45e2ed9

Updated plot3d.hxsurf for plotly plotting engine.

096e94b

Updated plot3d.cmtkreg for plotly plot engine.

4571682

Fixed check errors.

06af2d7

Updated return values of plotly handles.

435b5eb

Updated test cases for plotly engine.

e65b6d5

SridharJagannathan and others added 7 commits August 19, 2019 10:24

Created the default plot engine options.

6f9800e

Updated the plotengine as function argument.

10eb4ef

Fixed the errors in plotengine argument

1e292a3

Introduced opacity and exported clearplotlyscene funcion

b820d9c

Changed the opacity argument (plotly) to follow the alpha argumen (rgl).

9854383

code style

486d9be

jefferis reviewed Sep 6, 2019

View reviewed changes

jefferis added 4 commits September 6, 2019 23:05

plotly: rmarkdown support by removing explicit prints

507531f

* see #410 (comment) * add a plotly vignette to demonstrate this is actually the case

tidy up plot3d.boundingbox plotly implementationn

a2b1d62

Give plot3d.boundingbox plotly colour/lwd support

76a673a

* also reduce default line width to 1 for plotly

Teach plotly plot3d.hxsurf to identify meshes

5270caf

* this is something rather nice that we get with plotly

jefferis added 8 commits September 6, 2019 23:05

remove plotly_plotonce

0a2bed7

* no longer necessary * also standardise docs for plotengine

New nclear3d function (plotly/rgl)

e7879cf

nlscan is only implemented for rgl at present

686dadf

check plotengine setting

db4802d

* otherwise we could end up with a bad value and some stuff that checks for rgl not happening

Make a full draft vignette re 3d graphics

8f6ca68

Add trace names to somata of dotprops neurons

5bde8e8

* note that hover colour does not match soma unfortunately.

Update 3d Graphics vignette

7233706

* add to index

switch intro vignette to plotly

136c808

Fixed the issue of mesh visualization (range of the axes).

04ff657

Updated aspect ratio mode for plotly engine.

0797a48

jefferis merged commit 0797a48 into master Sep 25, 2019

jefferis deleted the feature/plotly branch February 3, 2020 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/plotly #410

Feature/plotly #410

SridharJagannathan commented Aug 17, 2019 •

edited

Loading

coveralls commented Aug 17, 2019 •

edited

Loading

jefferis commented Aug 17, 2019

jefferis commented Aug 17, 2019

jefferis commented Aug 27, 2019

jefferis commented Aug 27, 2019

jefferis commented Aug 27, 2019

jefferis commented Sep 6, 2019

jefferis Sep 6, 2019 •

edited

Loading

jefferis Sep 6, 2019 •

edited

Loading

jefferis Sep 6, 2019

jefferis commented Sep 11, 2019

SridharJagannathan commented Sep 13, 2019 •

edited

Loading

SridharJagannathan commented Sep 13, 2019

jefferis commented Sep 13, 2019

jefferis commented Sep 13, 2019

SridharJagannathan commented Sep 13, 2019

jefferis commented Sep 14, 2019 •

edited

Loading

SridharJagannathan commented Sep 19, 2019

SridharJagannathan commented Sep 19, 2019 •

edited

Loading

jefferis commented Sep 24, 2019

Feature/plotly #410

Feature/plotly #410

Conversation

SridharJagannathan commented Aug 17, 2019 • edited Loading

coveralls commented Aug 17, 2019 • edited Loading

jefferis commented Aug 17, 2019

jefferis commented Aug 17, 2019

jefferis commented Aug 27, 2019

jefferis commented Aug 27, 2019

jefferis commented Aug 27, 2019

jefferis commented Sep 6, 2019

jefferis Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

jefferis Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

jefferis Sep 6, 2019

Choose a reason for hiding this comment

jefferis commented Sep 11, 2019

SridharJagannathan commented Sep 13, 2019 • edited Loading

SridharJagannathan commented Sep 13, 2019

jefferis commented Sep 13, 2019

jefferis commented Sep 13, 2019

SridharJagannathan commented Sep 13, 2019

jefferis commented Sep 14, 2019 • edited Loading

SridharJagannathan commented Sep 19, 2019

SridharJagannathan commented Sep 19, 2019 • edited Loading

jefferis commented Sep 24, 2019

SridharJagannathan commented Aug 17, 2019 •

edited

Loading

coveralls commented Aug 17, 2019 •

edited

Loading

jefferis Sep 6, 2019 •

edited

Loading

jefferis Sep 6, 2019 •

edited

Loading

SridharJagannathan commented Sep 13, 2019 •

edited

Loading

jefferis commented Sep 14, 2019 •

edited

Loading

SridharJagannathan commented Sep 19, 2019 •

edited

Loading