Data importer #589

PavelBal · 2021-08-24T14:29:06Z

This PR implements the ImporterConfiguration and requires the Core version from this PR: Open-Systems-Pharmacology/OSPSuite.Core#1250

Importing data sets in R requires an excel file and a configuration. There are three ways to get a configuration in R:

Load an existing configuration (e.g. the one created in PK-Sim or MoBi)
Create a configuration from scratch
Create a configuration for a file/sheet (basically what PK-Sim tries to do when starting an import process). This is not implemented yet and will be done in a separate PR.

PavelBal · 2021-08-24T14:29:42Z

The failing tests should work again when the Core PR is merged.

PavelBal · 2021-08-24T14:31:00Z

Each configuration has at least a time and a measurement columns, so these are created when creating an empty configuration. An error column can be added or removed manually.

PavelBal · 2021-08-24T14:32:24Z

R/utilities-data-import-configuration.R

@@ -0,0 +1,2 @@
+createConfigurationForFile <- function(filePath) {


Will be implemented later.

PavelBal · 2021-08-24T14:32:34Z

R/utilities-pk-analysis.R

-  finally = {
-    file.remove(pkParameterResultsFilePath)
-  }
+  dataFrame <- tryCatch(


PavelBal · 2021-08-24T14:33:01Z

tests/data/dataImporterConfiguration.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="utf-8"?>


Added a couple of configurations created in PK-Sim for testing.

PavelBal · 2021-08-24T14:34:32Z

R/data-importer-configuration.R

+
+    #' @field timeUnit If \code{timeUnitFromColumn} is \code{FALSE}, unit of the values in time column
+    #' If \code{timeUnitFromColumn} is \code{TRUE}, name of the column with units of the values in time column
+    timeUnit = function(value) {


Time unit (the same is valid for measurement unit) - it is either a fixed unit that the user can change, or the name of the column that defines the units.

PavelBal · 2021-08-25T15:51:48Z

@msevestre ready to review

R/data-importer-configuration.R

msevestre

Looks very good already . a few minor changes
Specifically, I feel that we could simplify some if the if/else by returning early and or using some of the helper methods

msevestre · 2021-08-25T17:43:08Z

R/data-importer-configuration.R

+      validateIsString(value)
+      # Fixed unit or from column?
+      if (private$.isUnitFromColumn(column)) {
+        # do nothing as it should be NULL


return ? So that there is no else?

return(NULL) in fact

Returning invisible, as usually we do not return anything when trying to set a property.

hum.. but the line above, you return(NULL)

if (private$.isUnitFromColumn(column)) { return(NULL) }

Lines 67-74: getting measurementDimension (if (missing(value)) {). If the unit of measurement is defined in a column, the dimension is NULL as it will be set on import. Return NULL because getting a value should always return something.

Lines 75-93: setting measurementDimension. In case the unit is defined in a column, specifying a dimension does not make any sense, so the action should do nothing. However, the user also does not expect the action to return anything, that' s why I use return(invisible(self)) to quit early.

msevestre · 2021-08-25T17:44:11Z

R/data-importer-configuration.R

+      if (private$.isUnitFromColumn(column)) {
+        # do nothing as it should be NULL
+      } else {
+        validateDimension(enc2utf8(value))


lets create a variable for this
value = enc2utf8(value) and use it everywhere. A bit too many duplicate for my likinig

Yepp you are right.

msevestre · 2021-08-25T17:46:29Z

R/data-importer-configuration.R

+    errorColumn = function(value) {
+      column <- private$.errorColumn
+      if (missing(value)) {
+        if (is.null(column)) {


a cool method to simplify all those if
https://github.com/Open-Systems-Pharmacology/OSPSuite-R/blob/develop/R/utilities.R#L44

msevestre · 2021-08-25T17:47:07Z

R/data-importer-configuration.R

+        if (missing(value)) {
+          return(NULL)
+        }
+        return(invisible(self))


What does this mean here?isn't it somekind of weird configuration to set something but there is no column?

In case a configuration does not have an error column defined (this is a valid configuration), errorUnit and errorType are NULL. Trying to set errorUnit and errorType without defining an error column does not make sense, so it just does nothing (thus returning silently self), instead of throwing an error.

msevestre

one last question... I like the refactoring

msevestre · 2021-08-26T12:45:37Z

R/data-importer-configuration.R

+      validateIsString(value)
+      # Fixed unit or from column?
+      if (private$.isUnitFromColumn(column)) {
+        # do nothing as it should be NULL


hum.. but the line above, you return(NULL)

if (private$.isUnitFromColumn(column)) { return(NULL) }

…ure xml files are valid files

msevestre · 2021-08-26T19:17:53Z

Ok I am going to merge. This. The behavior with null vs invisible(self) is a bit unclear to me but these are edge cases anyways

codecov-commenter · 2021-08-26T19:20:32Z

Codecov Report

Merging #589 (9936fd4) into develop (39c6b78) will increase coverage by 0.43%.
The diff coverage is 95.23%.

❗ Current head 9936fd4 differs from pull request most recent head 7146e84. Consider uploading reports for the commit 7146e84 to get more accurate results

@@             Coverage Diff             @@
##           develop     #589      +/-   ##
===========================================
+ Coverage    89.85%   90.28%   +0.43%     
===========================================
  Files           74       75       +1     
  Lines         1686     1833     +147     
===========================================
+ Hits          1515     1655     +140     
- Misses         171      178       +7

Impacted Files	Coverage Δ
R/utilities-units.R	`94.11% <ø> (ø)`
R/data-importer-configuration.R	`95.23% <95.23%> (ø)`

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 39c6b78...7146e84. Read the comment docs.

PavelBal requested a review from msevestre August 24, 2021 14:29

PavelBal commented Aug 24, 2021

View reviewed changes

R/utilities-pk-analysis.R Outdated

finally = {

file.remove(pkParameterResultsFilePath)

}

dataFrame <- tryCatch(

Copy link

Member Author

PavelBal Aug 24, 2021

Choose a reason for hiding this comment

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

Run styler

PavelBal commented Aug 24, 2021

View reviewed changes

msevestre reviewed Aug 25, 2021

View reviewed changes

R/data-importer-configuration.R Outdated Show resolved Hide resolved

msevestre requested changes Aug 25, 2021

View reviewed changes

msevestre reviewed Aug 26, 2021

View reviewed changes

PavelBal and others added 12 commits August 26, 2021 14:59

DateImporter configuration WIP

ef6a50d

DataConfiguration

c1c01a7

Continue working on import configuration

bad8f7e

Fixes ImporterConfiguration from file

e115888

Styler

59ec178

Fix tests

4b61ee1

Respect encapsulation of UnitDescritpion

10dbc7f

Styler

69fb959

Addressing review comments

f537346

update dll

db7bc17

update dll

c5e514d

Make test pass again by removing unused variable obsData and making s…

9936fd4

…ure xml files are valid files

msevestre force-pushed the data-importer branch from 3e9808b to 9936fd4 Compare August 26, 2021 19:05

Also update documentaiton

7146e84

msevestre merged commit 89b6afa into develop Aug 26, 2021

msevestre deleted the data-importer branch August 26, 2021 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data importer #589

Data importer #589

PavelBal commented Aug 24, 2021

PavelBal commented Aug 24, 2021

PavelBal commented Aug 24, 2021

PavelBal Aug 24, 2021

PavelBal Aug 24, 2021

PavelBal Aug 24, 2021

PavelBal Aug 24, 2021

PavelBal commented Aug 25, 2021

msevestre left a comment

msevestre Aug 25, 2021

msevestre Aug 25, 2021

PavelBal Aug 26, 2021

msevestre Aug 26, 2021

PavelBal Aug 26, 2021

msevestre Aug 25, 2021

PavelBal Aug 26, 2021

msevestre Aug 25, 2021

msevestre Aug 25, 2021

PavelBal Aug 26, 2021

msevestre left a comment

msevestre Aug 26, 2021

msevestre commented Aug 26, 2021

codecov-commenter commented Aug 26, 2021

		@@ -0,0 +1,2 @@
		createConfigurationForFile <- function(filePath) {

Data importer #589

Data importer #589

Conversation

PavelBal commented Aug 24, 2021

PavelBal commented Aug 24, 2021

PavelBal commented Aug 24, 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

PavelBal commented Aug 25, 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre commented Aug 26, 2021

codecov-commenter commented Aug 26, 2021

Codecov Report