#4 add plot histogram #12

KatrinCoboeken · 2023-12-08T10:16:24Z

No description provided.

work on typos Open-Systems-Pharmacology#6 set up _pkgdown.yml (still buggy)

add plotBoxwhisker

work on typos Open-Systems-Pharmacology#6 set up _pkgdown.yml (still buggy)

add plotBoxwhisker

…eken/OSPSuite.Plots-ztcok into #4_add_plot_Histogram

Yuri05 · 2023-12-08T12:58:27Z

@KatrinCoboeken 2 tests failed...

R/MappedData.R

Yuri05 · 2023-12-08T13:33:14Z

R/MappedData.R

    translateErrorAestethics = function() {
      if (!private$aestheticExists(paste(private$direction, "min")) |
        !private$aestheticExists(paste(private$direction, "max"))) {
        errorType <-
-          intersect(names(self$mapping), c("error", "error_relativ"))
+          intersect(names(self$mapping), c("error", "error_relative"))


we should define constants for those strings (e.g. via the enumeration "ColumnNames" or whatsoever; compare
https://github.com/Open-Systems-Pharmacology/TLF-Library/blob/1a74ca24c2d318b5d1a6dc52f51121e1ca00c6c1/R/utilities-enums.R#L43-L48

not necessarily in this PR, but in general

Yuri05 · 2023-12-08T13:38:39Z

R/MappedDataTimeProfile.R

+      checkmate::assertDouble(
+        ylimits,
+        sorted = TRUE,
+        any.missing = TRUE,
+        len = 2,
+        unique = TRUE,
+        null.ok = TRUE
+      )


I have seen this function several times, and all arguments (except the first one) are the same.
Consider introducing the function with just 1 argument ("checkAxisLimits(limits)" or whatsoever)

Yuri05 · 2023-12-08T13:43:45Z

R/add_Layer.R

+initializePlot <- function(mappedData = NULL,
+                           setMapping = TRUE) {
+  # Validation
+  checkmate::assertClass(mappedData, classes = "MappedData", null.ok = TRUE)


for many of this checks, ospsuite.utils provides validation functions, e.g.
https://www.open-systems-pharmacology.org/OSPSuite.RUtils/reference/isOfType.html

That was an intended omission
I want that the functions of this package are used and maybe copied and adjusted by people, with R experience but not so much ospsuite experience. So if there was a common known R function I have used that instead of an opsuite function.

There are other reasons to use checkmate instead of the ospsuite.utils validation functions:

Why write our own function, if there is already a package which does this perfectly

with checkmate you can adress a list of validation topics (Type, any missing, kenght,..) in one line whereas you need in ospsuite utils one line per topic.

Checkmate is much faster (see attached screenshot):

Yuri05 · 2023-12-08T13:45:06Z

R/add_Layer.R

+
+  # check for geomUnicodeMode
+  geomUnicodeMode <- getOption(
+    x = "ospsuite.plots.GeomPointUnicode",


please always try to define constants for things like this

I do not think to save options as constants make sense. Options are already somethign like options.

I meant, define a constant for the string "ospsuite.plots.GeomPointUnicode" and other constant strings

Yuri05 · 2023-12-08T13:46:55Z

R/add_Layer.R

@@ -178,22 +244,31 @@ buildWatermarkGrob <- function(label, x = .5, y = .5, angle = 30,
 addXscale <- function(plotObject,
                      xscale,
                      xscale.args = list()) {
-  checkmate::assertChoice(xscale, choices = c("linear", "log"), null.ok = TRUE)
+  checkmate::assertChoice(xscale, choices = c("linear", "log", "discrete"), null.ok = TRUE)


Possible scales should be defined as enum, then validateEnumValue from ospuite.utils can be used

I can define scales as enum, but

I prefer checkmate to validateEnumValue

In this case you have to be careful, for some cases (like timeprofiles) only continuous x-scales like "linear" and "log" make sense other plotTypes l(like boxwhisker) accept also discrete

you can still use the checkmate (or whatsoever other validation) with enums and check for concrete values, e.g.
checkmate::assertChoice(xscale, choices = c(xScales$linear, xScales$log), null.ok = TRUE)

Yuri05 · 2023-12-08T13:47:40Z

R/plotBoxWhisker.R

+                           plotObject = NULL,
+                           percentiles = getOption(
+                             x = "ospsuite.plots.Percentiles",
+                             default = getDefaultOptions()[["ospsuite.plots.Percentiles"]]


define as constant

that was also about the string "ospsuite.plots.Percentiles"

Yuri05 · 2023-12-08T13:49:28Z

R/plotHistogram.R

+  )
+
+
+
+
+  #-  initialize plot
+  plotObject <- initializePlot(
+    mappedData
+  )
+
+
+  if (plotAsFrequency) {


why so many empty lines in between?

I wil fix it

R/sysdata.rda

R/utilities.R

tests/testthat/Rplots.pdf

Felixmil · 2023-12-08T14:54:16Z

DESCRIPTION

What is are the reasons the package Depends on ggplot2 and ggh4x?(i.e why Imports is not enough ?)

Import was not enough as I used shortcuts like 'axis_minor' for ggh4x::guide_axis_minor().
I changed all such short cuts and shifted ggh4x to Import. Now it should work

Felixmil · 2023-12-08T15:00:55Z

R/MappedDataBoxplot.R

+    #' @param mapping list of aesthetic mappings
+    #' @param groupAesthetics vector of aesthetics, which are used for columns mapped with aesthetic `groupby`
+    #' @param direction direction of plot either "x" or "y"
+    #' @param isObserved Flag if TRUE mappings mdv, lloq, error and error_relative are evaluated


I never saw Flag used in R packages or OSPS R packages to describe a boolean/logical. I can understand It but I'm not sure the users are really familiar with this naming.

Felixmil · 2023-12-08T15:25:32Z

tests/testthat/helper-for-tests.R

@@ -1,4 +1,4 @@
-# Helper for tests 
+# Helper for tests
 getTestDataFilePath <- function(fileName) {
  dataPath <- file.path(getwd(), "..", "data", fsep = .Platform$file.sep)


Please use testthat::test_path("data", fileName) instead, it will target tests/testthat/data/[filename].

As general rule, never use getwd() or setwd() in a package (and avoid it in general).

I will delete the whole file. It is not needed in ospsuite.plots.
I copied it in an early version of my package from the ospsuite.ReportingEngine package.

Felixmil · 2023-12-08T15:28:43Z

README.md

 -   [ggplot2](https://cran.r-project.org/package=ggplot2/index.html)
+-   [ggh4x](https://cran.r-project.org/package=ggh4x/index.html)


When installed from binary, all "Imports" and "Depends" packages will be required to be installed manually, see: https://github.com/Open-Systems-Pharmacology/OSPSuite-R#installation.

Maybe the installation part of README.Rmd from ospsuite package could be reused here.

- change Flag to boolean - add description for installation to README.md - delete tests/testthat/Rplots.pdf and add it to .gitingnore - correct typos - delete empty lines - shift ggh4x from depends to import - delete helperForTests

Merge branch '#4_add_plot_Histogram' of https://github.com/KatrinCoboeken/OSPSuite.Plots-ztcok into #4_add_plot_Histogram

- change Flag to boolean - add description for installation to README.md - delete tests/testthat/Rplots.pdf and add it to .gitingnore - correct typos - delete empty lines - shift ggh4x from depends to import - delete helperForTests replace labelsFOrpercentiles with scales::label_ordinal Merge branch '#4_add_plot_Histogram' of https://github.com/KatrinCoboeken/OSPSuite.Plots-ztcok into #4_add_plot_Histogram fix teastfail order of linetypes in plotratiovscov.svg by fixing order of linetyps

Yuri05 · 2023-12-12T13:21:41Z

All tests are running now, yeah 🙂

From the review comments, there are some remaining issues (s. below) - but I think those can be addressed in other PRs (I would create new issues then for them).

@KatrinCoboeken So if you are not going to address any of them right now, I would approve the PR.

generate README.md from README.Rmd
#4 add plot histogram #12 (review)
Define constants for strings
#4 add plot histogram #12 (review)
#4 add plot histogram #12 (review)
#4 add plot histogram #12 (review)
Avoid code duplication
#4 add plot histogram #12 (review)
Define enumerations where applicable
#4 add plot histogram #12 (review)
Too many empty lines
#4 add plot histogram #12 (review)

KatrinCoboeken · 2023-12-12T13:39:43Z

Thanks Juri, I will adress the open issues step by step in the next days for the next pull request.

KatrinCoboeken and others added 22 commits September 1, 2023 17:43

finalized first version of plotHistogram with Vignette

8bc1d35

avoid linter warnings Open-Systems-Pharmacology#7,

ae858d5

work on typos Open-Systems-Pharmacology#6 set up _pkgdown.yml (still buggy)

update spleling and style

c48bc27

add plotBoxwhisker

update OSPSuite vignettes, with user feedback

a660878

add PK Ratio plots

eb37fab

add Goodness of Fit plots add Vignettes

1e5ccbe

add tests

fb47f2d

update vignettes

c3249fa

switch for shape mode

088e472

Spelling and Lint checks

ce32ab3

finalized first version of plotHistogram with Vignette

eaa198c

avoid linter warnings Open-Systems-Pharmacology#7,

e2ec996

work on typos Open-Systems-Pharmacology#6 set up _pkgdown.yml (still buggy)

update spleling and style

5df76d4

add plotBoxwhisker

update OSPSuite vignettes, with user feedback

09c6da9

add PK Ratio plots

f391a59

add Goodness of Fit plots add Vignettes

f412416

add tests

03a9930

update vignettes

7b91ed2

switch for shape mode

fbcbd48

Spelling and Lint checks

a85f535

Merge branch '#4_add_plot_Histogram' of https://github.com/KatrinCobo…

c2fa2aa

…eken/OSPSuite.Plots-ztcok into #4_add_plot_Histogram

cleanup merging remainings

3e53a2e

Yuri05 reviewed Dec 8, 2023

View reviewed changes

R/MappedData.R Outdated Show resolved Hide resolved

Yuri05 reviewed Dec 8, 2023

View reviewed changes

R/sysdata.rda Outdated Show resolved Hide resolved

Yuri05 reviewed Dec 8, 2023

View reviewed changes

R/utilities.R Outdated Show resolved Hide resolved

Yuri05 reviewed Dec 8, 2023

View reviewed changes

tests/testthat/Rplots.pdf Outdated Show resolved Hide resolved

Yuri05 requested review from msevestre, Felixmil and pchelle December 8, 2023 14:06

Felixmil reviewed Dec 8, 2023

View reviewed changes

KatrinCoboeken added 4 commits December 11, 2023 09:10

insert feedback from pull request:

4eea9b2

- change Flag to boolean - add description for installation to README.md - delete tests/testthat/Rplots.pdf and add it to .gitingnore - correct typos - delete empty lines - shift ggh4x from depends to import - delete helperForTests

replace labelsFOrpercentiles with scales::label_ordinal

57938af

Merge branch '#4_add_plot_Histogram' of https://github.com/KatrinCoboeken/OSPSuite.Plots-ztcok into #4_add_plot_Histogram

fix order of linetypes in plotYVsX

f9de1e2

Yuri05 approved these changes Dec 12, 2023

View reviewed changes

This was referenced Dec 12, 2023

Create README.md from README.Rmd #13

Open

Define constants for strings #14

Open

Avoid code duplication #15

Open

Define enumerations where applicable #16

Open

Too many empty lines #17

Open

Yuri05 merged commit 53bd60b into Open-Systems-Pharmacology:develop Dec 12, 2023
2 checks passed

KatrinCoboeken mentioned this pull request Dec 13, 2023

add goodness of fit plots #11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4 add plot histogram #12

#4 add plot histogram #12

KatrinCoboeken commented Dec 8, 2023

Yuri05 commented Dec 8, 2023

Yuri05 Dec 8, 2023

Yuri05 Dec 8, 2023

Yuri05 Dec 8, 2023 •

edited

Loading

Yuri05 Dec 8, 2023 •

edited

Loading

KatrinCoboeken Dec 8, 2023

KatrinCoboeken Dec 11, 2023

Yuri05 Dec 8, 2023

KatrinCoboeken Dec 11, 2023

Yuri05 Dec 11, 2023

Yuri05 Dec 8, 2023

KatrinCoboeken Dec 11, 2023

Yuri05 Dec 11, 2023

Yuri05 Dec 8, 2023

Yuri05 Dec 11, 2023

Yuri05 Dec 8, 2023

KatrinCoboeken Dec 11, 2023

Felixmil Dec 8, 2023 •

edited

Loading

KatrinCoboeken Dec 11, 2023

Felixmil Dec 8, 2023 •

edited

Loading

Felixmil Dec 8, 2023 •

edited

Loading

KatrinCoboeken Dec 11, 2023

Felixmil Dec 8, 2023

Yuri05 commented Dec 12, 2023

KatrinCoboeken commented Dec 12, 2023 via email •

edited

Loading

		- [ggplot2](https://cran.r-project.org/package=ggplot2/index.html)
		- [ggh4x](https://cran.r-project.org/package=ggh4x/index.html)

#4 add plot histogram #12

#4 add plot histogram #12

Conversation

KatrinCoboeken commented Dec 8, 2023

Yuri05 commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuri05 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Yuri05 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felixmil Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felixmil Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Felixmil Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuri05 commented Dec 12, 2023

KatrinCoboeken commented Dec 12, 2023 via email • edited Loading

Yuri05 Dec 8, 2023 •

edited

Loading

Yuri05 Dec 8, 2023 •

edited

Loading

Felixmil Dec 8, 2023 •

edited

Loading

Felixmil Dec 8, 2023 •

edited

Loading

Felixmil Dec 8, 2023 •

edited

Loading

KatrinCoboeken commented Dec 12, 2023 via email •

edited

Loading