Skip to content

Lineplot@main#166

Open
BFalquet wants to merge 5 commits intomainfrom
lineplot@main
Open

Lineplot@main#166
BFalquet wants to merge 5 commits intomainfrom
lineplot@main

Conversation

@BFalquet
Copy link

This pull request introduces a new set of utilities for generating line plots with summary statistics and tables, along with comprehensive documentation and extensive tests. The changes add functions for preprocessing data, calculating statistics, and creating publication-ready plots and tables using ggplot2, all supported by both example-based and automated tests.

The most important changes are:

New line plot and statistics utilities:

  • Added export statements for g_lineplot_table, g_lineplot_with_table, and g_lineplot_without_table to the package namespace, making these functions available for use.
  • Introduced new documentation for the following functions, providing detailed usage, arguments, and examples:

Testing improvements:

  • Added a new simple test script (test-gg_lineplot-simple.R) that runs basic functionality checks for all new plot and statistics functions, including data creation, function outputs, and expected columns.
  • Added a comprehensive test suite (test-gg_lineplot.R) using testthat to validate all new functions, including edge cases (e.g., NA handling, custom statistics functions, confidence intervals, decimal places, and grouped/ungrouped data).

These additions significantly enhance the package's capabilities for statistical line plotting and ensure reliability through robust testing.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 13, 2026


🎉 Thank you for your contribution! Before this PR can be accepted, we require that you read and agree to our Contributor License Agreement.
You can digitally sign the CLA by posting a comment on this Pull Request in the format shown below. This agreement will apply to this PR as well as all future contributions on this repository.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Comment on lines +1 to +13
# Copyright (c) 2024 Roche
# SPDX-License-Identifier: MIT

#' @title Simple tests for gg_lineplot functions
#' @description Basic tests that can run without full package installation

# Load required libraries
if (!require("ggplot2")) install.packages("ggplot2")
if (!require("dplyr")) install.packages("dplyr")
if (!require("tidyr")) install.packages("tidyr")

# Load the functions
source("R/gg_lineplot.R")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be deleted?

Comment on lines +15 to +17
# Helper function to create test data
create_test_data <- function() {
set.seed(123)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid functions for test data and random seed generator by setting some values directly (I use gemini)

Comment on lines +21 to +25
# Test calc_stats function
#' @description Test calc_stats function
#' @param x Numeric vector
#' @param conf_level Confidence level
#' @param decimal_places Number of decimal places
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very confusing here ahah

Comment on lines +27 to +28
set.seed(456)
x <- rnorm(100, mean = 10, sd = 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

As above we do not need so many data points, we can directly write them w/o the seed

# Test default parameters
result <- calc_stats(x)
expect_is(result, "list")
expect_names(result, c("n", "mean", "mean_ci", "mean_ci_lwr", "mean_ci_upr", "median", "sd"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me tern... We should follow some other naming like gtsummary style

Comment on lines +6 to +9
#' @param x A numeric vector.
#' @param conf_level The confidence level for the interval (default is 0.95).
#' @param decimal_places Number of decimal places for numeric values in the table.
#' @return A list with statistics: `n`, `mean`, `mean_ci_lwr`, `mean_ci_upr`, `median`, `sd`.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing descriptions with our style (list)\cr etc..

#' @return A list with statistics: `n`, `mean`, `mean_ci_lwr`, `mean_ci_upr`, `median`, `sd`.
#' @export
#' @examples
#' set.seed(123)
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not need seed as we do not test against

#' set.seed(123)
#' x <- rnorm(100, mean = 10, sd = 2)
#' calc_stats(x, conf_level = 0.95, decimal_places = 2)
calc_stats <- function(x, conf_level = 0.95, decimal_places = 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe there is out there a simple stats function that does this. We should not rely on our own (as cardx does)

Comment on lines +334 to +357
g_lineplot_with_table <- function(df_stats,
x = "AVISIT",
group_var = "ARM",
strata_N = NULL,
mid = "mean",
whiskers = c("mean_ci_lwr", "mean_ci_upr"),
table = NULL,
mid_type = "pl",
mid_point_size = 2,
position = position_dodge(width = 0.4),
legend_title = NULL,
legend_position = "bottom",
ggtheme = nestcolor::theme_nest(),
x_lab = NULL,
y_lab = NULL,
title = "Plot of Mean and 95% Confidence Limits by Visit",
subtitle = "",
caption = NULL,
table_font_size = 3,
decimal_places = 2,
errorbar_width = 0.45,
col = NULL,
linetype = NULL,
rel_height_plot = 0.5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These are way too many variables. The majority of these details are never modified, and we should keep them set before-hand. We can add options to change them quite easily afterwards if really requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants