8000 Fix 817 - Add HTMLWidgets barcharts by mattroumaya · Pull Request #883 · Gilead-BioStats/gsm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix 817 - Add HTMLWidgets barcharts #883

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 67 commits into from
Dec 5, 2022
Merged

Fix 817 - Add HTMLWidgets barcharts #883

merged 67 commits into from
Dec 5, 2022

Conversation

mattroumaya
Copy link
Contributor
@mattroumaya mattroumaya commented Nov 16, 2022 •
edited
Loading

Fix #817

Overview

Ported rbm-viz into gsm:

  • Implemented a first pass in AE_Assess()
  • Added JS libraries to inst
  • Created R wrapper barChart.R
  • Plots are available when running:
ae <- AE_Map_Raw() %>% AE_Assess()

Test Notes/Sample Code

workflow_to_plot <- "kri0001"


snapshot <- Make_Snapshot()

results <- snapshot$results_summary %>% 
  dplyr::filter(workflowid == workflow_to_plot)

workflow <- snapshot$meta_workflow %>% 
  dplyr::filter(workflowid == workflow_to_plot)

threshold <- snapshot$meta_param %>% 
  dplyr::filter(param == "vThreshold", workflowid == workflow_to_plot)

yaxis <- 'score'

barChart(
  data = results,
  config = workflow,
  threshold = threshold,
  yaxis = yaxis,
  elementId = "test"
)

To do:

  • It seems that the thresholds from Make_Snapshot() aren't working when passed to barChart()
  • Need to work out details on creating/maintaining the threshold and config arguments for barChart()
  • Need to discuss overall implementation and make decisions about overall implementation:
    • do we keep ggplot in *_Assess()?
    • How/where to include in Study_Report()?
    • Naming conventions? (just tacked on JS to the plot name for now)

@mattroumaya mattroumaya marked this pull request as ready for review November 30, 2022 16:36
samussiah
samussiah requested changes Dec 5, 2022
View reviewed changes
Copy link
Contributor
@samussiah samussiah left a comment

Choose a reason for hiding this comment

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

This looks awesome @MayaGans and @mattroumaya! Left some minor comments but otherwise everything's working wonderfully on my end.

I am going to babel-ify rbm-viz so the widgets work natively in RStudio on my end. I don't know what's different about my environment but transpiling should do the trick.

R/PD_Assess.R
if (strMethod != "identity") {
lCharts$scatter <- gsm::Visualize_Scatter(dfFlagged = lData$dfFlagged, dfBounds = lData$dfBounds, strGroupLabel = strGroup)
if (!bQuiet) cli::cli_alert_success("{.fn Visualize_Scatter} created {length(lCharts)} chart.")

if (exists('dfBounds', lData)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed in addition to line 178? It's in every _Assess function except AE_Assess.

R/AE_Assess.R
if (!bQuiet) cli::cli_alert_success("{.fn Visualize_Scatter} created {length(lCharts)} chart.")

# rbm-viz charts ----------------------------------------------------------
lCharts$scatterJS <- scatterPlot(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer Widget or Interactive here if I may be so pedantic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samussiah will update the naming in a separate PR, think that'll make things a tad easier

mattroumaya and others added 3 commits December 5, 2022 11:30
@mattroumaya @samussiah
Update R/AE_Assess.R
6a11117 
Co-authored-by: Spencer Childress <spencer.c.childress@gmail.com>
@mattroumaya @samussiah
Update R/Consent_Assess.R
fe4bdc9 
Co-authored-by: Spencer Childress <spencer.c.childress@gmail.com>
@mattroumaya @samussiah
Update R/Disp_Assess.R
a18468b 
Co-authored-by: Spencer Childress <spencer.c.childress@gmail.com>
@samussiah samussiah self-requested a review December 5, 2022 20:58
@mattroumaya mattroumaya merged commit 6abd7e3 into dev Dec 5, 2022
@mattroumaya mattroumaya deleted the add-barchart branch December 5, 2022 22:20
@samussiah samussiah mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samussiah samussiah samussiah approved these changes

@jwildfire jwildfire Awaiting requested review from jwildfire

Assignees

@MayaGans MayaGans

@mattroumaya mattroumaya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Migrate charts to rbm-viz widgets.
4 participants
@mattroumaya @samussiah @MayaGans @Zhongkai-Wang
0