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

Issue114 #339

Merged
merged 8 commits into from
Sep 20, 2024
Merged

Issue114 #339

merged 8 commits into from
Sep 20, 2024

Conversation

nhall6
Copy link
Collaborator

@nhall6 nhall6 commented Sep 18, 2024

Fixing several things outlined in #114

Standardizing power table outputs to adhere to OSM standard theme (reactable via resultTableViewer and resultTableServer). Also split the tables into 2 sub-tabs for easier readability
Adding conditional formatting of colors to values to aid in interpretation, and also adding an absolute value of beta column.
Overlaying equipoise statistic onto the plot, and truncating cohort names in legend
Adding maxSDM to covariate balance plot
Adding to caption descriptions in figures and overlaying EASE statistic on systematic error plot
Adding more descriptive filenames for plot downloads
@nhall6 nhall6 added bug Something isn't working enhancement New feature or request fixed in develop labels Sep 18, 2024
@nhall6 nhall6 requested a review from jreps September 18, 2024 15:30
@jreps
Copy link
Collaborator

jreps commented Sep 19, 2024

R check is showing:
"─ checking R code for possible problems ... [11s/13s] NOTE (13.4s)
cohortMethodPropensityModelServer : : no visible binding for
global variable ‘coefficient’
Undefined global functions or variables:
coefficient
"

This normally means a missing .data$ before coefficient in the code. Please add this.

@jreps
Copy link
Collaborator

jreps commented Sep 19, 2024

R check is also showing multiple errors:
Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-estimation-cohort-method-covariateBalance.R:3'): (code run outside of test_that()) ──
Error in paste0("Max SDM Statistic = ", maxSdmStatistic): argument "maxSdmStatistic" is missing, with no default
Backtrace:

  1. ├─shiny::testServer(...) at test-estimation-cohort-method-covariateBalance.R:3:0
  2. │ ├─shiny:::withMockContext(...)
  3. │ │ ├─shiny::isolate(...)
  4. │ │ │ ├─shiny::..stacktraceoff..(...)
  5. │ │ │ └─ctx$run(...)
  6. │ │ │ ├─promises::with_promise_domain(...)
  7. │ │ │ │ └─domain$wrapSync(expr)
  8. │ │ │ ├─shiny::withReactiveDomain(...)
  9. │ │ │ │ └─promises::with_promise_domain(...)
  10. │ │ │ │ └─domain$wrapSync(expr)
  11. │ │ │ │ └─base::force(expr)
  12. │ │ │ └─env$runWith(self, func)
  13. │ │ │ └─shiny (local) contextFunc()
  14. │ │ │ └─shiny::..stacktraceon..(expr)
  15. │ │ ├─shiny::withReactiveDomain(...)
  16. │ │ │ └─promises::with_promise_domain(...)
  17. │ │ │ └─domain$wrapSync(expr)
  18. │ │ │ └─base::force(expr)
  19. │ │ └─withr::with_options(...)
  20. │ │ └─base::force(code)
  21. │ └─rlang::eval_tidy(quosure, mask, rlang::caller_env())
  22. ├─OhdsiShinyModules:::plotCohortMethodCovariateBalanceScatterPlotNew(...) at test-estimation-cohort-method-covariateBalance.R:55:4
  23. │ └─... %>% ...
  24. ├─plotly::layout(...)
  25. ├─plotly:::layout.plotly(...)
  26. └─base::paste0("Max SDM Statistic = ", maxSdmStatistic)
    ── Error ('test-estimation-cohort-method-power.R:3'): (code run outside of test_that()) ──
    Error in .subset2(x, "impl")$getOutput(name): The test referenced an output that hasn't been defined yet: output$proxy1-powerTable
    Backtrace:
  27. ├─shiny::testServer(...) at test-estimation-cohort-method-power.R:3:0
  28. │ ├─shiny:::withMockContext(...)
  29. │ │ ├─shiny::isolate(...)
  30. │ │ │ ├─shiny::..stacktraceoff..(...)
  31. │ │ │ └─ctx$run(...)
  32. │ │ │ ├─promises::with_promise_domain(...)
  33. │ │ │ │ └─domain$wrapSync(expr)
  34. │ │ │ ├─shiny::withReactiveDomain(...)
  35. │ │ │ │ └─promises::with_promise_domain(...)
  36. │ │ │ │ └─domain$wrapSync(expr)
  37. │ │ │ │ └─base::force(expr)
  38. │ │ │ └─env$runWith(self, func)
  39. │ │ │ └─shiny (local) contextFunc()
  40. │ │ │ └─shiny::..stacktraceon..(expr)
  41. │ │ ├─shiny::withReactiveDomain(...)
  42. │ │ │ └─promises::with_promise_domain(...)
  43. │ │ │ └─domain$wrapSync(expr)
  44. │ │ │ └─base::force(expr)
  45. │ │ └─withr::with_options(...)
  46. │ │ └─base::force(code)
  47. │ └─rlang::eval_tidy(quosure, mask, rlang::caller_env())
  48. ├─testthat::expect_true(!is.null(output$powerTable)) at test-estimation-cohort-method-power.R:52:4
  49. │ └─testthat::quasi_label(enquo(object), label, arg = "object")
  50. │ └─rlang::eval_bare(expr, quo_get_env(quo))
  51. ├─output$powerTable
  52. └─shiny:::$.shinyoutput(output, powerTable)
  53. └─.subset2(x, "impl")$getOutput(name)
    ── Error ('test-estimation-cohort-method-systematicError.R:68'): plotCohortMethodScatter ──
    Error in paste0("EASE Statistic = ", ease): argument "ease" is missing, with no default
    Backtrace:
  54. └─OhdsiShinyModules:::plotCohortMethodScatter(controlResults) at test-estimation-cohort-method-systematicError.R:68:2
  55. ├─ggplot2::ggtitle(paste0("EASE Statistic = ", ease))
  56. │ └─ggplot2::labs(title = label, subtitle = subtitle)
  57. │ └─rlang::dots_list(...)
  58. └─base::paste0("EASE Statistic = ", ease)

[ FAIL 3 | WARN 24 | SKIP 1 | PASS 451 ]

  • Pleased fix these errors and confirm tests all pass by running an R check locally.

jreps and others added 2 commits September 19, 2024 13:35
* Fixing #301 (#326)

Adding alphabetical sort to DB input options

* Fixing #296 (#327)

Standardizing pickerInput type for database selection where multiple options are possible to improve UX. Also fixed some colDefs in Exposed Cases

* Fixing #302 and Filtered Data Downloads (#332)

* Fixing #302 and Filtered Data Downloads

All filtered data downloads across all modules should work now, with a button styled the same as the full download. It needs to be a CSV handler though due to the reactable statte

* Updating the fix

Also fixed download buttons in Cohorts module, addressing #298

* Update R_CMD_check_Hades.yaml

* Update R_CMD_check_Hades.yaml

* Update R_CMD_check_Hades.yaml

* fixing R checks

fixing R checks

* Update cohort-diagnostics-databaseInformation.R (#333)

Fixing the issue reported in #162

* fixing issue issue_330 (#334)

fixing time plot x-axis

* fixing issue 167 (#335)

added code to get long database names on multiple lines

* Update cohort-diagnostics-timeDistributions.R (#336)

adding fix for issue 168

* Update patient-level-prediction-modelSummary.R (#337)

---------

Co-authored-by: Nathan Hall <106178605+nhall6@users.noreply.github.com>
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 39 lines in your changes missing coverage. Please review.

Project coverage is 77.70%. Comparing base (0a22d6c) to head (e996799).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
R/characterization-riskFactors.R 50.00% 12 Missing ⚠️
R/estimation-cohort-method-propensityModel.R 60.00% 10 Missing ⚠️
R/helpers-sccsPlots.R 0.00% 7 Missing ⚠️
R/cohort-diagnostics-cohort-overlap.R 0.00% 3 Missing ⚠️
R/components-data-viewer.R 83.33% 3 Missing ⚠️
R/estimation-cohort-method-covariateBalance.R 97.77% 1 Missing ⚠️
R/estimation-cohort-method-power.R 99.24% 1 Missing ⚠️
...mation-cohort-method-propensityScoreDistribution.R 97.61% 1 Missing ⚠️
R/estimation-cohort-method-systematicError.R 98.03% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #339      +/-   ##
===========================================
+ Coverage    77.60%   77.70%   +0.10%     
===========================================
  Files           78       78              
  Lines        20627    20946     +319     
===========================================
+ Hits         16007    16277     +270     
- Misses        4620     4669      +49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jreps jreps merged commit 0d8f845 into develop Sep 20, 2024
8 checks passed
@jreps jreps deleted the issue114 branch September 20, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request fixed in develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants