Fix inconsistent legend aesthetics #63

nautilus69 · 2025-11-27T16:22:28Z

when plotting observed and simulated data there are inconsistency on the legend this will resolve #57

Harmonize guide_legend order values (Simulated: order=2, Observed: order=1)
Standardize legend title capitalization (Observed instead of observed)
Use guides() function consistently instead of scale_shape(guide = none)
Add test for observedMapping parameter with different aesthetics

…ed data (#57) - Harmonize guide_legend order values (Simulated: order=2, Observed: order=1) - Standardize legend title capitalization (Observed instead of observed) - Use guides() function consistently instead of scale_shape(guide = none) - Add test for observedMapping parameter with different aesthetics

rengelke

The fix is solid and could be merged as is.

However, to close issue #57 we need to address the inconsistent aesthetic grouping.

Since the issue arose during integration of ospsuite.plots into ospsuite, and was not very specific, it might be worth taking a look at the integration test snapshots in ospsuite to identify all aesthetic/legend problems at once and specify them.
Understanding the full scope of aesthetic inconsistencies, might help to create a comprehensive solution at this point.

Would you be open to test this branch against ospsuite's test suite (test-plot-with-ospsuite-plots-individualTimeProfiles.R, test-plot-with-ospsuite-plots-population-time-profile.R) and document what we find? Desired vs obtained plots. Again, this might help to get a clearer picture.

nautilus69 · 2025-11-28T13:16:40Z

yes I will update it again, in order to pass those test with correcting snapshots.

Copilot

Pull request overview

This PR fixes inconsistent legend aesthetics when plotting observed and simulated data together. The changes standardize legend ordering, title capitalization, and the method for controlling legend display.

Key changes:

Changed legend order values to display Observed (order=1) before Simulated (order=2)
Standardized legend title capitalization from "observed" to "Observed"
Replaced scale_shape(guide = "none") with guides(shape = "none") for consistency

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File	Description
R/plotTimeProfile.R	Renamed parameters from camelCase to dot.case with backward compatibility support, fixed legend ordering (Simulated: order=2, Observed: order=1), standardized legend title capitalization, and replaced scale_shape with guides()
tests/testthat/test-plotTimeProfile.R	Added test case for plotTimeProfile with different observedMapping parameter to verify legend consistency

Copilot · 2025-12-18T09:21:19Z

R/plotTimeProfile.R

+#' @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


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.

I agree with copilot

Copilot · 2025-12-18T09:21:19Z

R/plotTimeProfile.R

 #' @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


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 · 2025-12-18T09:21:19Z

R/plotTimeProfile.R

+#' @param y2scale.args list with arguments for secondary y-axis
 #' @param requireDualAxis boolean if TRUE secondary axis is needed
-#' @param yScaleArgs list with y2scale arguments
+#' @param yscale.args list with y2scale arguments


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 · 2025-12-18T09:21:19Z

R/plotTimeProfile.R

 #'  `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


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 · 2025-12-18T09:21:20Z

R/plotTimeProfile.R

+      guidesList <- stats::setNames(
+        list(guide_legend(title = "Simulated", order = 2)),
+        aesthetic
+      )
      plotObject <- plotObject +
-        guides(aesthetic = guide_legend(title = "observed", order = 1)) +
+        guides(!!!guidesList) +


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

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

Copilot · 2026-01-06T10:00:32Z

R/plotTimeProfile.R

+      guidesList <- stats::setNames(
+        list(guide_legend(title = "Simulated", order = 2)),
+        aesthetic
+      )
      plotObject <- plotObject +
-        guides(aesthetic = guide_legend(title = "observed", order = 1)) +
+        guides(!!!guidesList) +
        ggnewscale::new_scale(new_aes = aesthetic)


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 · 2026-01-06T10:00:32Z

R/plotTimeProfile.R

                            geomErrorbarAttributes = getDefaultGeomAttributes("Errorbar"),
                            geomLLOQAttributes = getDefaultGeomAttributes("LLOQ"),
                            groupAesthetics = c("colour", "fill", "shape")) {
+


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

KatrinCoboeken

Looks good to me — great work!

nautilus69 requested review from KatrinCoboeken and rengelke November 27, 2025 16:37

rengelke requested changes Nov 28, 2025

View reviewed changes

nautilus69 marked this pull request as draft November 28, 2025 13:16

optimized guide() location and mapSimulatedAndObserved updated

8dbde57

Copilot AI review requested due to automatic review settings December 18, 2025 09:20

Copilot AI reviewed Dec 18, 2025

View reviewed changes

fixing camelCase and updated Roxygen

93e2f2a

nautilus69 marked this pull request as ready for review January 6, 2026 09:59

nautilus69 requested review from Copilot and rengelke January 6, 2026 09:59

Copilot AI reviewed Jan 6, 2026

View reviewed changes

KatrinCoboeken approved these changes Jan 7, 2026

View reviewed changes

Fix inconsistent legend aesthetics #63

Are you sure you want to change the base?

Fix inconsistent legend aesthetics #63

Uh oh!

Conversation

nautilus69 commented Nov 27, 2025

Uh oh!

rengelke left a comment

Choose a reason for hiding this comment

Uh oh!

nautilus69 commented Nov 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

KatrinCoboeken Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

KatrinCoboeken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants