add checkboxes for dose response curves for log transformation and increasing assumption#153
add checkboxes for dose response curves for log transformation and increasing assumption#153tonywu1999 merged 4 commits intodevelfrom
Conversation
📝 WalkthroughWalkthroughThis change introduces new namespace constants for response-curve fitting options and plot visualization controls, refactors plot rendering from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/module-statmodel-server.R (1)
433-449:⚠️ Potential issue | 🟡 MinorProvide a fallback default for the
heightparameter ingroupComparisonPlotscalls.The
heightslider is conditionally rendered only whenplot_type == Heatmap(line 843–844), and even then it doesn't exist on the first render. For Volcano and ComparisonPlot types, the slider never exists. This meansinput[[NAMESPACE_STATMODEL$visualization_plot_height_slider]]will beNULLin both cases. While MSstats::groupComparisonPlots has a defaultheight = 600, passingNULLexplicitly does not trigger that default and will likely cause an error.Apply the fix to both line 433 and line 449:
Proposed fix
- height = input[[NAMESPACE_STATMODEL$visualization_plot_height_slider]], + height = input[[NAMESPACE_STATMODEL$visualization_plot_height_slider]] %||% 500,(Using
%||%from rlang, orif (!is.null(...)) ... else 500if rlang is unavailable)
🧹 Nitpick comments (3)
R/statmodel-ui-options-modeling.R (1)
17-17: Stale comment — the feature it requests is now implemented by this PR.The comment
# need option for increasing or decreasing trend for dose responsedescribes exactly what this PR adds. Consider removing it.Proposed fix
- # need option for increasing or decreasing trend for dose responseR/module-statmodel-server.R (2)
797-848: Nestedrender*insiderenderUI— functional but worth noting the trade-offs.Creating
renderPlot/renderPlotlyoutputs insiderenderUI(lines 802, 814, 829) produces anonymous outputs. This works in Shiny, but has a caveat: every time therenderUIre-evaluates (e.g., whenvisualization_view_resultschanges), the anonymous outputs are destroyed and recreated, losing any client-side state.Two additional notes on this block:
Line 810:
matrix <- contrast$matrixshadowsbase::matrix. Not a bug here but a readability concern — consider renaming to e.g.contrast_mat.Line 819:
ratio_response = TRUEinvisualizeResponseProtein, whilefitResponseCurves(utils.R:1313) usesratio_response = FALSE. If these are intentionally different (fitting vs. visualization), a brief comment explaining why would help future maintainers.
836-847: Height slider is created inside the samerenderUIthat reads its value.The slider for heatmap height is emitted as part of the
renderUIoutput, but the plot (output_plot) already readinput[[visualization_plot_height_slider]]during this same render pass. On the very first render the slider doesn't exist yet so height isNULL. Once the slider appears, changes to it will re-triggerrenderPlotly(since it's a reactive dependency inside the render function), so subsequent interactions work fine — it's only the initial render that getsNULL.If you want the slider to drive height from the start, consider placing the
sliderInputin a separaterenderUIor in the static UI, rather than co-locating it with the plot output.
Summary by CodeRabbit