el-meyer / airship

AIRSHIP - An Interactive R-SHIny apP for visualizing tidy long data
https://el-meyer.github.io/airship/
GNU General Public License v3.0
3 stars 2 forks source link

Bug with plotly #148

Open el-meyer opened 9 months ago

el-meyer commented 9 months ago

When dataset is very large and interactive plot is chosen, it results in a serious break of the app, RStudio and the browser it is run in. Can be reproduced with ExampleDataFacts and Scatterplot using the full dataset (225k rows). Seems to be a known issue.

@berryni @majkamichal What do you think? Should we have a server-side check that disables this feature if the dataset is larger than, say, 100k rows? Would be cool to find a way where the option is just greyed out and when hovered over, says "Your dataset is too large to enable interactive features".

el-meyer commented 9 months ago

The previous commit basically adds this functionality, but for some reason the interactive plot is now empty. The new lines are therefore commented out. Need to revise.

majkamichal commented 9 months ago

I agree that the best trade-off is to check for dataset size (either in terms of rows or size in megabytes) and disable entering to the interactive mode.

switchInput can be disabled by adding disabled=TRUE

shinyWidgets::updateSwitchInput( inputId = shiny::NS(cID, "bPlotly"), disabled = TRUE, ...)

I will try to code it asap.

el-meyer commented 9 months ago

With my previous commit and uncommenting the lines, it correctly warns the user of too large dataset and disables plotly when the dataset is too large:

image

However, when the dataset is small enough and plotly is chosen, the plot is empty:

image

majkamichal commented 9 months ago

Please see my commit.

The solution is to have a new module that is enabling or disabling the toogle for Plotly if a certain threshold of data is exceeded.

It works well. However, the tooltip is not working for some reason and I don't have time to investigate it further.

For some reason, I had to pass a global session variable to updateSwitchInput so that it is correctly updated...that is very strange.

Maybe a different approach would be better:

el-meyer commented 9 months ago

This only disables the interactive features once when the dataset is uploaded and checked for row-number.

In our case, it needs to be dynamic based on the focus variables and aesthetics the user has chosen, because it is easily possible that the dataset initially is 300k rows long, but after setting focus variables and displaying only 2 aesthetics in the plot, it is 40k rows and interactivitiy should be possible. I have tried some adaptations to your commit, namely:

-) replacing the call to data_prefiltered() with e.g. boxplot_get()$lData -) switching from disabling the interactivity toggle to simply setting it to FALSE

but then I get the same problems I had with my initial commit

el-meyer commented 9 months ago

I will experiment a bit further, in a first attempt I will try replacing the SwitchInput with a regular checkboxInput.

EDIT: That did not solve the problem.

majkamichal commented 9 months ago

When I change it to listening to boxplot_get()$lData it works well for me. Please see the third commit for #148.

You can see number of rows after dynamic filtering.

It works well for both, boxplot and scatterplot.

However, when I do the same with ldplot, the app crashes because of pivot longer in airship:::fnDynFilterData: Warning: Error in tidyr::pivot_longer:colsmust select at least one column.

One would need to make the function responsible for dynamic filtering more robust.


fnDisablePlotlyToggleServer(cID = "boxplot", data = reactive({ print("boxplot") print(nrow(boxplot_get()$lData)) boxplot_get()$lData }), global_session = session)

el-meyer commented 9 months ago

The function for dynamic borrowing is more robust now and the error for LDPlot disappeared.

However, the whole thing is still very buggy: I just crashed the Boxplot with a few clicks:

image

Maybe we need to take a step back and for now disable Plotly based on the initial dataset that is uploaded to make sure that the app is useable without crashes and re-visit this issue at a later point. What do you think @majkamichal ?

el-meyer commented 9 months ago

Interestingly, the susceptibility of plotly to fail has nothing to do with these commits. I just crashed it on the latest stable release using the same process.

EDIT: The "process" is very simple: Toggle plotly on and off very quickly.

el-meyer commented 9 months ago

To have a version that does not crash, I will leave all the infrastructure from these commits in place and merge them into master, but disable plotly globally for the time being in fnTogglePlotly.