WIP #544 #546

msevestre · 2021-07-02T13:52:50Z

No description provided.

msevestre · 2021-07-02T13:53:17Z

This is just the beginning... and it should not be merged at all

msevestre · 2021-07-02T13:53:53Z

R/data-repository.R

-
-      private$throwPropertyIsReadonly("baseGrid")
+      private$.baseGrid <- value
+      private$wrapProperty("BaseGrid", value$ref)


Exposing now some of the Readonly properties

msevestre · 2021-07-02T13:54:30Z

R/data-set.R

+    }
+  ),
+  public = list(
+    dataRepository = NULL,


Public for now so that I can debug stuff. It will be private once the implementation is stable

msevestre · 2021-07-02T13:54:59Z

R/data-set.R

+      self$dataRepository$name <- value
+    },
+    #' @field xUnit Unit in which the xValues are defined
+    xUnit = function(value){


This is not the real implementation. Just dummies for now for all methods

msevestre · 2021-07-02T13:55:27Z

R/data-set.R

+      # Create an empty data repository with a base grid and column
+      dataRepository <- DataRepository$new()
+      # Passing time for dimension for now
+      xValues <- DataColumn$new(rClr::clrNew("OSPSuite.Core.Domain.Data.BaseGrid", "xValues", getDimensionByName("Time")))


I think it makes sense to initialize with some kind of defaults (time, and conc for instance)

I will use our tasks to get the name instead of using magic string

msevestre · 2021-07-02T19:36:43Z

@PavelBal Tagging @hannaei as well here for review

msevestre

@hannaei @PavelBal @Yuri05
Please review. I think this is in a good place for @hannaei or @PavelBal to continue now. I have added capabilities in core to update dimension and unit

msevestre · 2021-07-02T19:37:06Z

R/data-column.R

    values = function(value) {
-      private$wrapReadOnlyProperty("ValuesAsArray", value)
+      private$wrapProperty("ValuesAsArray", value)


I updated core to support setting values as well.

msevestre · 2021-07-02T19:37:44Z

R/data-column.R

+      # we use the ignore case parameter set  to true so that we do not have to worry about casing when set via scripts
+      unit <-  rClr::clrCall(dimension, "FindUnit", value, TRUE)
+      if(is.null(unit)){
+        stop(messages$errorUnitNotSupported(unit = value, dimension = self$dimension))


Right now, an error will be thrown. This is OK in the DataColun object. In the DataSet one, we could be a bit more user friendly (see comment below)

msevestre · 2021-07-02T19:38:34Z

R/data-column.R

+      }
+      # updating the dimension
+      rClr::clrSet(self$ref, "Dimension", getDimensionByName(value))
+      private$.dimension <- NULL


For efficient usage, we are caching dimension and unit. We need to reset the cache obviously when updating the dimension

msevestre · 2021-07-02T19:39:24Z

R/data-set.R

+      if (missing(value)) {
+        return(private$.xValues$dimension)
+      }
+      private$setColumnDimension(private$.xValues, value)


I have define a method for each seta ction so that we can call it for x, y and later error

msevestre · 2021-07-02T19:40:04Z

R/data-set.R

+    #' @description
+    #' Print the object to the console
+    #' @param ... Rest arguments.
+    print = function(...) {


@hannaei and @PavelBal
Here the print method should be enhanced to show the data in a user friendly fashion. I'll leave that to you

msevestre · 2021-07-02T19:40:45Z

R/data-set.R

+      values <- private$getColumnValues(column)
+
+      #now we need to update dimension and display unit
+      column$displayUnit <- unit


HERE: If the unit is not supported, we could easily update the dimension by using the method getDimensionForUnit. That way, the user would never have to worry about updating the dimension when updating the unit.

@msevestre I would not prefer to automatically change the dimension, as I see it as a sanity check here. Simply changing the unit only affects what actual values the data have, but not the meaning of the data. Changing the dimension actually means that the data means something else. What do you think?

msevestre · 2021-07-02T19:41:22Z

R/data-set.R

+    initializeCache = function() {
+      private$.xValues <- self$dataRepository$baseGrid
+      # TODO we need to be a bit more careful here
+      private$.yValues <- self$dataRepository$allButBaseGrid[[1]]


There is a more to do. Specifically regarding the error column.

codecov-commenter · 2021-07-02T19:50:23Z

Codecov Report

Merging #546 (e5624f4) into develop (1832b04) will decrease coverage by 0.12%.
The diff coverage is 84.15%.

❗ Current head e5624f4 differs from pull request most recent head 45587b6. Consider uploading reports for the commit 45587b6 to get more accurate results

@@             Coverage Diff             @@
##           develop     #546      +/-   ##
===========================================
- Coverage    88.61%   88.49%   -0.13%     
===========================================
  Files           72       74       +2     
  Lines         1449     1625     +176     
===========================================
+ Hits          1284     1438     +154     
- Misses         165      187      +22

Impacted Files	Coverage Δ
R/messages.R	`100.00% <ø> (ø)`
R/object-base.R	`100.00% <ø> (ø)`
R/utilities-data-set.R	`0.00% <0.00%> (ø)`
R/data-repository.R	`88.37% <90.00%> (+4.37%)`	⬆️
R/data-column.R	`95.45% <94.73%> (+81.16%)`	⬆️
R/data-set.R	`94.82% <94.82%> (ø)`
R/dot-net-wrapper.R	`92.85% <100.00%> (+0.35%)`	⬆️
R/utilities-units.R	`93.82% <100.00%> (+0.23%)`	⬆️
R/utilities.R	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1832b04...45587b6. Read the comment docs.

PavelBal · 2021-07-05T15:02:45Z

@msevestre Should we remove "utilities-data-mapping" and completely replace it by "utilities-data-set"? E.g. loadDataRepositoryFromPKML would become loadDataSetFromPKML and create a data set object.

PavelBal · 2021-07-05T15:03:51Z

Should we maybe fork this so we can merge PRs? I do not want to have a huge PR without intermediate states.

* Continue working on DataSet * Update man on DataColumn * WIP DataSet adding (failing) test

msevestre

@PavelBal I think this is good to review ...

msevestre · 2021-07-27T21:54:24Z

R/data-repository.R

+    #'
+    #' @param name Name of new meta data list entry
+    #' @param value Value of new meta data list entry
+    addMetaData = function(name, value) {


@PavelBal Add and remove meta data as separate function in data repo.

there was this comment
#' If \code{value} is \code{NULL}, the entry with corresponding name is deleted from meta data set.

That I don't want to have. Let's create two methods instead

msevestre · 2021-07-27T21:55:38Z

R/data-repository.R

+      validateIsString(name)
+      validateIsString(value)
+      dataRepositoryTask <- getNetTask("DataRepositoryTask")
+      rClr::clrCall(dataRepositoryTask, "AddMetaData", self$ref, name, value)


is there a way to coherce a parameter to string in R. If yes, then we can remove the need for value to be a string

as.character() works e.g. for numerics but not for class objects, so maybe leave the check here.

msevestre · 2021-07-27T21:56:20Z

R/data-set.R

+    #' When changing from arithmetic to geometric error, the values are considered in as fraction (1 = 100%).
+    #' When changing from geometric to arithmetic, the values are set to the same unit as \code{yErrorUnit}.
+    yErrorType = function(value) {
+      private$.createErrorColumnIfMissing()


when accessing Error properties, I create the column if it's not there. The way it was implemented @PavelBal , you would modify the original repo if loaded from file and I don't think this is pure

I don't really understand... Now the original repo is modified the first time any property of yError is called, no?

msevestre · 2021-07-27T21:58:00Z

R/utilities.R

+#'
+#' @return A string containing the name of the enumerated constant in enumType whose value is enumValue; or null if no such constant is found.
+netEnumName <- function(enumType, enumValue){
+  netTypeObj <- rClr::clrGetType(enumType)


@PavelBal also fixed this

msevestre · 2021-07-27T21:58:29Z

tests/testthat/test-data-set.R

+  expect_equal(dataSet$yErrorValues, numeric(0))
+})
+
+test_that("it can print a data set", {


@PavelBal Also fixed this

msevestre · 2021-07-30T12:29:33Z

R/utilities-data-set.R

@@ -0,0 +1,46 @@
+
+makeColumn <- function(dataSet, property) {


This function is intended to be private it seems. Let make it start with a "."

msevestre · 2021-07-30T12:31:29Z

R/utilities-data-set.R

+#' @return DataSet objects as dataframe with columns name, xValue, yValue, yErrorValues,
+#' xDimension, xUnit, yDimension, yUnit, yErrorType, yErrorUnit, yMolWeight
+#' @export
+dataSetToDataFrame <- function(dataSets) {


we need tests for this method, at least just to make sure that this does not crash with updated DataSet class

msevestre · 2021-08-04T15:12:54Z

@PavelBal DLL updated to support the RemoveColumn

Merge branch '544-data-set' of https://github.com/Open-Systems-Pharmacology/OSPSuite-R into 544-data-set

PavelBal · 2021-08-05T14:52:48Z

@msevestre I think this is ready. Tests for "toDataFrame" are WIP and can be added in another PR, I think.

WIP #544

68c928a

msevestre requested review from PavelBal and Yuri05 July 2, 2021 13:53

msevestre commented Jul 2, 2021

View reviewed changes

WIP #544 data set

f0e7fb1

Update test-data-set.R

11a0718

msevestre commented Jul 2, 2021

View reviewed changes

PavelBal and others added 5 commits July 27, 2021 11:31

Wip data set (#564)

14a5ab7

* Continue working on DataSet * Update man on DataColumn * WIP DataSet adding (failing) test

Make tests pass again

22c4ca6

Fixes #544 data set update

98239a2

Fixes #544 data set update

b5b1deb

Fixes #544 data set update

6df6c8c

msevestre marked this pull request as ready for review July 27, 2021 21:49

msevestre added 2 commits July 27, 2021 17:52

Rebase changes on develop

eb80a1f

Rebase changes on develop

dab9f43

msevestre commented Jul 27, 2021

View reviewed changes

Johanna Eitel added 2 commits July 30, 2021 13:06

Added utilities-data-set.R and property molWeight to DataSet

b6e7eb8

Added utilities-data-set.R

d7ad8b0

msevestre commented Jul 30, 2021

View reviewed changes

msevestre added 2 commits July 30, 2021 08:36

Fixes failing test

f717962

Bump version 301 of ospsuite.core

e2d2ded

PavelBal added 3 commits August 5, 2021 14:44

Add tests for data set

5f5c62b

Add removing y error

e5624f4

Merge branch '544-data-set' of https://github.com/Open-Systems-Pharmacology/OSPSuite-R into 544-data-set

Styler

45587b6

msevestre merged commit 7139c40 into develop Aug 5, 2021

msevestre deleted the 544-data-set branch August 5, 2021 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP #544 #546

WIP #544 #546

msevestre commented Jul 2, 2021

msevestre commented Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre commented Jul 2, 2021

msevestre left a comment

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

msevestre Jul 2, 2021

PavelBal Jul 5, 2021

msevestre Jul 2, 2021

codecov-commenter commented Jul 2, 2021 •

edited

Loading

PavelBal commented Jul 5, 2021

PavelBal commented Jul 5, 2021

msevestre left a comment

msevestre Jul 27, 2021

msevestre Jul 27, 2021

msevestre Jul 27, 2021

PavelBal Jul 28, 2021

msevestre Jul 27, 2021

PavelBal Jul 28, 2021

msevestre Jul 27, 2021

msevestre Jul 27, 2021

msevestre Jul 30, 2021

msevestre Jul 30, 2021

msevestre commented Aug 4, 2021

PavelBal commented Aug 5, 2021

WIP #544 #546

WIP #544 #546

Conversation

msevestre commented Jul 2, 2021

msevestre commented Jul 2, 2021

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

msevestre commented Jul 2, 2021

msevestre left a 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

codecov-commenter commented Jul 2, 2021 • edited Loading

Codecov Report

PavelBal commented Jul 5, 2021

PavelBal commented Jul 5, 2021

msevestre left a 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

msevestre commented Aug 4, 2021

PavelBal commented Aug 5, 2021

codecov-commenter commented Jul 2, 2021 •

edited

Loading