Improve plot_ly

[william-hutchison]

Multiple changes to plot_ly were made, to summarise:

• The plot_ly methods were switched from S3 to S4.
• A plot_ly example was added.
• The plot_ly man page was rename from plotly to plot_ly.
• Redundant plot_ly method definitions were removed, now that the generic method can be correctly loaded from tidySummarizedExperiment due to changes implemented in https://github.com/stemangiola/tidySummarizedExperiment/pull/83

[stemangiola]

Is the plot_ly method S4?

[stemangiola]

• The plot_ly man page was rename from plotly to plot_ly.

Does this break old code? if so, a proper deprecation should be implemented. Such as I have done in the tidy ecosystem before. you can read about deprecation for tidyverse.

[william-hutchison]

Is the plot_ly method S4?

No, plot_ly as exported by plotly is neither S3 or S4. It is a base type function with no associated methods or classes. tidySummarisedExperiment originally created an S3 generic to enable different classes to call the correct plot_ly method. I have switched to S4 to better handle the sharing of methods and the generic between tidySummarizedExperiment, tidySingleCellExperiment and tidySpatialExperiment.

Does this break old code? if so, a proper deprecation should be implemented. Such as I have done in the tidy ecosystem before. you can read about deprecation for tidyverse.

No, I only changed the name used in the documentation. Previously, the documentation name was plotly which does not match the name of the actual function plot_ly. An alias did exist for plot_ly, so if a user looked for help with ?plot_ly they would see the correct documentation. But in the package plotly, the function plotly is depreciated and is for a different purpose https://github.com/plotly/plotly.R/blob/270d20e21723fc9f5b09f3c1b4e80b9cf984ecfc/man/plotly.Rd. Making these change reduces complexity and better aligns with plotly.

I think the only way these changes could break anyone's code is if they had had written something explicitly checking the plot_ly function type, like:

if (isS4(plot_ly)) {
    ...
}

But I don't think there would ever be a reason to do this.

[stemangiola]

I see in this case, similarly to what happens for bind_rows, we put this generic method in the package ttservice, so the generics is created only once.

so I would say, let's do exactly what has been done for bind_rows. You need to submit ttservice to CRAN first and then once it is accepted (1,2, days) change tidy* packages accordingly.

[william-hutchison]

Sure, I have created a fork of the ttservice mirror https://github.com/william-hutchison/ttservice and added a S3 generic for plot_ly and a default method (copied from tidySingleCellExperiment and featuring the drop_class function used there). I do not have access to the CRAN repository, would you be able to copy my changes across, or provide me with access in some way?

Once the changes go through to CRAN I will resubmit pull requests to tidySingleCellExperiment and tidySummarizedExperiment to use the new generic in ttservice.

Also, because of these changes, I think tidySummarizedExperiment and tidySpatialExperiment should be added as ttservice reverse dependencies.

[stemangiola]

Sure, I have created a fork of the ttservice mirror https://github.com/william-hutchison/ttservice and added a S3 generic for plot_ly and a default method (copied from tidySingleCellExperiment and featuring the drop_class function used there). I do not have access to the CRAN repository, would you be able to copy my changes across, or provide me with access in some way?

Once the changes go through to CRAN I will resubmit pull requests to tidySingleCellExperiment and tidySummarizedExperiment to use the new generic in ttservice.

Also, because of these changes, I think tidySummarizedExperiment and tidySpatialExperiment should be added as ttservice reverse dependencies.

Great,

I think it would be much better to form my ttservice repository, so it's easier for me to merge your changes. Well that will be done. I will Maroochydore changes and submit to CRAN, when I will be back. Are we sure you how to submit to CRAN.

[william-hutchison]

Hi Stefano. I have been looking into how I can replicate the structure of bind_cols / bind_rows across ttservice, tidySummarizedExperiment ect. for plot_ly, and I think I have spotted a problem.

To use tidySummarisedExperiment's bind_cols as an example, the issue is that the S3 generic bind_cols needs to be imported from ttservice, while the base function bind_cols needs to be imported from dplyr. Because these are both needed they should both be imported. But, currently only bind_cols from ttservice is imported. Accessing bind_cols from dplyr replies on dplyr being loaded for different imports and then accesses the function without documenting the import. If the line #' @importFrom dplyr bind_cols is added to tidySummarisedExperiemnt bind_cols_internal to document the fact that this function need access to the function at line 67 dplyr::bind_cols(tts[[2]], .id=.id) %>%a warning is produced Warning: replacing previous import ‘dplyr::bind_cols’ by ‘ttservice::bind_cols’ when loading ‘tidySummarizedExperiment’

This same issue would be encountered if I tried to implement plot_ly in this way. The simplest solution would be import a different function from plotly to load the package and then silently import plot_ly from plotly, only documenting importing the generic plot_ly from ttservice. But this would definitely not be good practice.

I think it should be possible to use S4 to better manage the importing and exporting of the generic and methods. If you are happy with this change I can try to place a S4 generic for plot_ly in ttservice and replace the S3 methods with S4 methods. This would be similar to my original pull request except instead of tidySingleCellExperiment ect depending on tidySummarisedExperiment they will only depend on ttservice.

[william-hutchison]

Another option could be to import the base function plot_ly from plotly into ttservice, and then export it with a different name for use in tidySingleCellExperiment ect alongside an S3 generic. This could be a better option as it would minimise the changes required.

[stemangiola]

@william-hutchison I believe now ttservice has ben updated on CRAN

https://cran.r-project.org/web/packages/ttservice/index.html