Skip to content

Fix inconsistent legend aesthetics #63

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

Merged
nautilus69 merged 3 commits into from
Jan 16, 2026
Merged

Fix inconsistent legend aesthetics #63

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbbaf16
Fix inconsistent legend aesthetics when plotting observed and simulat…
Nov 27, 2025
8dbde57
optimized guide() location and mapSimulatedAndObserved updated
nautilus69 Dec 18, 2025
93e2f2a
fixing camelCase and updated Roxygen
nautilus69 Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 63 additions & 33 deletions R/plotTimeProfile.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@
#' @param geomErrorbarAttributes A `list` with arguments which are passed on to the call `ggplot2::geom_errorbar`
#' @param geomLLOQAttributes A `list` with arguments which are passed on to the call `ggplot2::geom_hline`
#' @param groupAesthetics vector of aesthetics, which are used for columns mapped with `groupby`,
#' @param xScale either 'linear' then `ggplot2::scale_x_continuous()` or 'log' then `ggplot2::scale_x_log10()` is used
#' @param xScaleArgs list of arguments passed to `ggplot2::scale_x_continuous()` or `ggplot2::scale_x_log10()`
#' @param yScale either 'linear' then `ggplot2::scale_y_continuous()` or 'log' then `ggplot2::scale_y_log10()` is used
#' @param yScaleArgs list of arguments passed to `ggplot2::scale_y_continuous()` or `ggplot2::scale_y_log10()`
#' @param y2Scale either 'linear' the secondary axis is displayed linear, or 'log' secondary axis is displayed with log scale
#' @param y2ScaleArgs list of arguments passed to `ggplot2::sec_axis()`, trans, break are set by code
#' @param xscale either 'linear' then `ggplot2::scale_x_continuous()` or 'log' then `ggplot2::scale_x_log10()` is used
#' @param xscale.args list of arguments passed to `ggplot2::scale_x_continuous()` or `ggplot2::scale_x_log10()`
#' @param yscale either 'linear' then `ggplot2::scale_y_continuous()` or 'log' then `ggplot2::scale_y_log10()` is used
#' @param yscale.args list of arguments passed to `ggplot2::scale_y_continuous()` or `ggplot2::scale_y_log10()`
#' @param y2scale either 'linear' the secondary axis is displayed linear, or 'log' secondary axis is displayed with log scale
#' @param y2scale.args list of arguments passed to `ggplot2::sec_axis()`, trans, break are set by code
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The parameter names in the roxygen documentation have been changed from camelCase (xScale, xScaleArgs, etc.) to dot.case (xscale, xscale.args, etc.). However, this violates the OSP R coding standards which specify that function and variable names should use camelCase. While backward compatibility is maintained through deprecated parameters, the new primary parameter names should follow the established camelCase convention.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Collaborator

@KatrinCoboeken KatrinCoboeken Dec 18, 2025

Choose a reason for hiding this comment

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

I agree with copilot

#'
#' @return A `ggplot` object
#' @examples
Expand Down Expand Up @@ -65,31 +65,57 @@ plotTimeProfile <- function(data = NULL, # nolint
observedMapping = mapping,
metaData = NULL,
mapSimulatedAndObserved = NULL,
xScale = AxisScales$linear,
xScaleArgs = list(limits = c(0, NA)),
yScale = AxisScales$linear,
yScaleArgs = list(),
y2Scale = AxisScales$linear,
y2ScaleArgs = list(),
xscale = AxisScales$linear,
xscale.args = list(limits = c(0, NA)),
yscale = AxisScales$linear,
yscale.args = list(),
y2scale = AxisScales$linear,
y2scale.args = list(),
plotObject = NULL,
geomLineAttributes = getDefaultGeomAttributes("Line"),
geomRibbonAttributes = getDefaultGeomAttributes("Ribbon"),
geomPointAttributes = getDefaultGeomAttributes("Point"),
geomErrorbarAttributes = getDefaultGeomAttributes("Errorbar"),
geomLLOQAttributes = getDefaultGeomAttributes("LLOQ"),
groupAesthetics = c("colour", "fill", "shape")) {
groupAesthetics = c("colour", "fill", "shape"),
xScale = NULL,
xScaleArgs = NULL,
yScale = NULL,
yScaleArgs = NULL,
y2Scale = NULL,
y2ScaleArgs = NULL) {
# Handle backward compatibility for camelCase parameter names
if (!is.null(xScale)) {
xscale <- xScale
}
if (!is.null(xScaleArgs)) {
xscale.args <- xScaleArgs
}
if (!is.null(yScale)) {
yscale <- yScale
}
if (!is.null(yScaleArgs)) {
yscale.args <- yScaleArgs
}
if (!is.null(y2Scale)) {
y2scale <- y2Scale
}
if (!is.null(y2ScaleArgs)) {
y2scale.args <- y2ScaleArgs
}

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

Unnecessary blank line added at the beginning of the function body. This adds no value and should be removed to maintain clean code formatting.

Suggested change

Copilot uses AI. Check for mistakes.
groupAesthetics <- ggplot2::standardise_aes_names(groupAesthetics)
.validatePlotTimeProfileInputs(
data = data,
observedData = observedData,
plotObject = plotObject,
metaData = metaData,
xScale = xScale,
xScaleArgs = xScaleArgs,
yScale = yScale,
yScaleArgs = yScaleArgs,
y2Scale = y2Scale,
y2ScaleArgs = y2ScaleArgs,
xScale = xscale,
xScaleArgs = xscale.args,
yScale = yscale,
yScaleArgs = yscale.args,
y2Scale = y2scale,
y2ScaleArgs = y2scale.args,
geomLineAttributes = geomLineAttributes,
geomRibbonAttributes = geomRibbonAttributes,
geomPointAttributes = geomPointAttributes,
Expand All @@ -106,11 +132,11 @@ plotTimeProfile <- function(data = NULL, # nolint
observedData = observedData,
observedMapping = observedMapping,
metaData = metaData,
xScale = xScale,
yScale = yScale,
yScaleArgs = yScaleArgs,
y2Scale = y2Scale,
y2ScaleArgs = y2ScaleArgs,
xScale = xscale,
yScale = yscale,
yScaleArgs = yscale.args,
y2Scale = y2scale,
y2ScaleArgs = y2scale.args,
groupAesthetics = groupAesthetics,
mapSimulatedAndObserved = mapSimulatedAndObserved
)
Expand All @@ -120,9 +146,9 @@ plotTimeProfile <- function(data = NULL, # nolint
simMappedData = listMappedData$simMappedData,
obsMappedData = listMappedData$obsMappedData,
plotObject = plotObject,
xScale = xScale,
xScaleArgs = xScaleArgs,
yScale = yScale,
xScale = xscale,
xScaleArgs = xscale.args,
yScale = yscale,
yScaleArgs = listMappedData$yScaleArgs, # for y2scaling, limits are updated
y2ScaleArgs = listMappedData$y2ScaleArgs,
secAxis = listMappedData$secAxis
Expand Down Expand Up @@ -185,7 +211,7 @@ plotTimeProfile <- function(data = NULL, # nolint
is.null(listMappedData$mapSimulatedAndObserved) &&
all(c("colour", "shape") %in% groupAesthetics)) {
plotObject <- plotObject +
scale_shape(guide = "none")
guides(shape = "none")
}

return(plotObject)
Expand Down Expand Up @@ -373,14 +399,14 @@ plotTimeProfile <- function(data = NULL, # nolint
#' @param simMappedData object of class `MappedDataTimeprofile` for simulated data
#' @param obsMappedData object of class `MappedDataTimeprofile` for observed data
#' @param commonLimits common limits for simulated and observed data
#' @param y2ScaleArgs list with arguments for secondary y-axis
#' @param y2scale.args list with arguments for secondary y-axis
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Inconsistent parameter name format in roxygen documentation. The parameter is documented as 'y2scale.args' (dot.case) but should be 'y2ScaleArgs' (camelCase) to comply with OSP R coding standards.

Copilot generated this review using guidance from repository custom instructions.
#' @param requireDualAxis boolean if TRUE secondary axis is needed
#' @param yScaleArgs list with y2scale arguments
#' @param yscale.args list with y2scale arguments
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Incorrect parameter description: 'yscale.args' is described as 'list with y2scale arguments' when it should describe arguments for the primary y-axis scale, not the secondary y2-axis. Additionally, the parameter name should follow camelCase convention (yScaleArgs) per OSP coding standards.

Copilot generated this review using guidance from repository custom instructions.
#'
#' @return list with
#' `simMappedData` adjusted object of class `MappedDataTimeprofile` for simulated data
#' `obsMappedData` adjusted object of class `MappedDataTimeprofile` for observed data
#' `yScaleArgs` adjusted `yScaleArgs` with common limits for primary and secondary y-axis
#' `yscale.args` adjusted `yscale.args` with common limits for primary and secondary y-axis
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Parameter name in return value documentation uses dot.case format 'yscale.args' which violates OSP R coding standards requiring camelCase naming (should be 'yScaleArgs').

Copilot generated this review using guidance from repository custom instructions.
#' `secAxis` secondary axis object
#'
#' @keywords internal
Expand Down Expand Up @@ -541,7 +567,7 @@ plotTimeProfile <- function(data = NULL, # nolint
plotObject <- plotObject +
scale_discrete_manual(aesthetic,
values = mapSimulatedAndObserved[[aesthetic]],
guide = guide_legend(order = 1, title = "Simulated")
guide = guide_legend(order = 2, title = "Simulated")
)
}
}
Expand Down Expand Up @@ -573,8 +599,12 @@ plotTimeProfile <- function(data = NULL, # nolint
# to separate simulated and observed legend entries and start again with default colors
if (!is.null(mapSimulatedAndObserved)) {
for (aesthetic in groupAesthetics) {
guidesList <- stats::setNames(
list(guide_legend(title = "Simulated", order = 2)),
aesthetic
)
plotObject <- plotObject +
guides(aesthetic = guide_legend(title = "observed", order = 1)) +
guides(!!!guidesList) +
Comment on lines +577 to +582
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

This code block creates a single-element list for one aesthetic inside a loop over multiple groupAesthetics. This approach differs from the similar code at lines 191-200 which correctly uses lapply to create guides for all aesthetics at once. Consider refactoring to match that pattern for consistency and to avoid repeatedly calling guides() inside the loop.

Copilot uses AI. Check for mistakes.
ggnewscale::new_scale(new_aes = aesthetic)
Comment on lines +577 to 583
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

This code creates a guides list for simulated data when it should be creating one for observed data. The title says 'Simulated' but this section is meant to set up the observed data legend before adding new scale. The title should be 'Observed' and the order should be 1, not 2.

Copilot uses AI. Check for mistakes.
}
}
Expand Down
Loading
Loading