Utilities dimensions #495

PavelBal · 2021-03-25T14:24:04Z

@msevestre No tests so far.

PavelBal · 2021-03-25T14:25:40Z

R/utilities-dimensions.R

+#' of both dimensions.
+#' If the values cannot be transformed between the dimensions, an error is thrown.
+#' @export
+dimensionsConversionFactor <- function(dimension1, dimension2, mw = NULL) {


This is very useful if you want e.g. to plot your data in molar or mass concentrations. Is there a better way to check if the dimensions can be converted than to explicitly consider every possible use case?

PavelBal · 2021-03-25T14:26:08Z

Maybe some things can be done better (using some .NET methods)?

PavelBal · 2021-03-25T15:19:20Z

Something is broken here..

msevestre · 2021-03-25T16:33:22Z

@PavelBal I have merge develop into the code here. Still it won't work but we have a clean slate

DESCRIPTION

msevestre · 2021-03-25T16:34:48Z

R/utilities-dimensions.R

+#' #' @include enum.R
+#' #'
+#' #' @export
+#' Dimensions <- enum(allAvailableDimensions())


This is a problem here. It is being loaded directly before initialization. This works when you load ospsuite.core first but does not work in ospsuite.core

msevestre · 2021-03-25T16:35:42Z

R/utilities-dimensions.R

+  validateUnit(fromUnit, dimension)
+  validateUnit(toUnit, dimension)
+
+  dimensionTask <- getDimensionTask()


I am not sure why you need sthg like this ? toUnit converts already everything from you. .The goal is to abstract exactly all of this away

The idea is to use the conversion capabilities of OSPS without having the simulation objects (quantities). E.g. read the data from excel and plot it in another unit. toUnit requires a Quantity.

nope

toUnit <- function(quantityOrDimension, values, targetUnit, molWeight = NULL) {

Specifically the parameter is called quantityOrDimension.

None of the proposed changes regarding reimplementing conversion by hand belong in Core. Helper function to ensure that dimension and unit etc.. exists, yes. The rest probably not :)

msevestre · 2021-03-25T16:35:55Z

R/utilities-dimensions.R

+#' of both dimensions.
+#' If the values cannot be transformed between the dimensions, an error is thrown.
+#' @export
+dimensionsConversionFactor <- function(dimension1, dimension2, mw = NULL) {


oh my. what is this monster?

msevestre · 2021-03-25T16:36:00Z

R/utilities-dimensions.R

+      if (is.null(mw)) {
+        stop(messages$errorCannotConvertDimensionsNoMW(dimension1, dimension2))
+      }
+      return(1 * 1e-6 * mw * 1e-3)


This is not something I want to see here. I need to understand the usage of such concept. We have already everything encapsulated in Core

Would be happy to see another way. E.g. - I read data in mg/dl and want to convert it to mol/l. How? Without having a Quantity object.

There are plenty of method exposted directly in core doing this. Creating a conversation factor from one dimension to another dimension does not make sense BTW. It needs to be from one unit to another unit

In core see

double[] ConvertToUnit(string dimensionName, string targetUnit, double[] valuesInBaseUnit, double molWeight); double[] ConvertToUnit(string dimensionName, string targetUnit, double[] valuesInBaseUnit); double[] ConvertToUnit(string dimensionName, string targetUnit, double valueInBaseUnit, double molWeight); double[] ConvertToUnit(string dimensionName, string targetUnit, double valueInBaseUnit);

and

double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double[] valuesInDisplayUnit, double molWeight); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double[] valuesInDisplayUnit); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double valueInDisplayUnit, double molWeight); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double valueInDisplayUnit);

All exposed via the DImensionTag. IT deals EXPLICITELY with those use cases

msevestre · 2021-03-25T16:37:04Z

R/utilities-dimensions.R

+#'
+#' @return String name of the base unit.
+#' @export
+getBaseUnit <- function(dimension) {


This should move to core

msevestre · 2021-03-25T16:37:18Z

R/utilities-dimensions.R

+  dimensionTask <- getDimensionTask()
+  # Get the dimension object
+  dimension <- rClr::clrCall(dimensionTask, "DimensionByName", enc2utf8(dimension))
+  unit <- rClr::clrCall(dimension, "get_BaseUnit")


there is clrGet...

Changed to

unit <- rClr::clrGet(dimension,") "BaseUnit

msevestre · 2021-03-25T16:38:15Z

R/utilities-dimensions.R

+#' @param dimension String name of the dimension.
+#' @details Check if the provided dimension is supported. If not, throw an error
+#' @export
+validateDimension <- function(dimension) {


should not we have one method (validateDimension or existDimension with a flag instead of two?) we have the same concept already somewhere else

Could you show where? I only remember the concept of "isXXX" returning a boolean, and "validateXXX" throwing an error if "isXXX" is false:

isOfType and validateIsOfType

msevestre · 2021-03-25T16:38:34Z

R/utilities-dimensions.R

+#' @details Check if the unit is valid for the dimension.
+#' @export
+hasUnit <- function(unit, dimension) {
+  validateIsString(c(unit, dimension))


This should move ot core too

What do you mean?

msevestre

This sitll needs to be moved to utilities-units

msevestre · 2021-03-25T20:02:46Z

R/utilities-dimensions.R

+#' @param dimension String name of the dimension.
+#' @details Returns \code{TRUE} if the provided dimension is supported otherwise \code{FALSE}
+#' @export
+hasDimension <- function(dimension) {


@PavelBal I have renamed this to hasDimension and it is using the core method

Why do you prefer the core method? We still have the enum Dimensions (or do you think we do not need it? I would disagree) which is created by calling allAvailableDimensions.

Why is it called HasDimension btw? What does or does not have the dimension? :D @msevestre

You are looking at this from the system point of view " Does OSPSUITE HAVE The dimension xxx"
so if(ospsuite::hasDimension("Mass")) reads better than if(ospsuite::existsDImension("Mass")) in my opinion

But this is also because we had hasUnit. and not existsUnit. Naming is hard and consistency is even harder

msevestre · 2021-03-25T20:02:54Z

R/utilities-dimensions.R

+  validateIsString(unit)
+  validateDimension(dimension)
+  dimensionTask <- getDimensionTask()
+  rClr::clrCall(dimensionTask, "HasUnit", enc2utf8(dimension),  enc2utf8(unit))


usign hasUnit from core

msevestre · 2021-03-25T20:03:06Z

R/utilities-dimensions.R

+getBaseUnit <- function(dimension) {
+  validateDimension(dimension)
+  dimensionTask <- getDimensionTask()
+  rClr::clrCall(dimensionTask, "BaseUnitFor", enc2utf8(dimension))


using baseUnitFor from core

msevestre · 2021-03-25T20:05:34Z

R/utilities-dimensions.R

+      if (is.null(mw)) {
+        stop(messages$errorCannotConvertDimensionsNoMW(dimension1, dimension2))
+      }
+      return(1 * 1e-6 * mw * 1e-3)


There are plenty of method exposted directly in core doing this. Creating a conversation factor from one dimension to another dimension does not make sense BTW. It needs to be from one unit to another unit

In core see

double[] ConvertToUnit(string dimensionName, string targetUnit, double[] valuesInBaseUnit, double molWeight); double[] ConvertToUnit(string dimensionName, string targetUnit, double[] valuesInBaseUnit); double[] ConvertToUnit(string dimensionName, string targetUnit, double valueInBaseUnit, double molWeight); double[] ConvertToUnit(string dimensionName, string targetUnit, double valueInBaseUnit);

and

double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double[] valuesInDisplayUnit, double molWeight); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double[] valuesInDisplayUnit); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double valueInDisplayUnit, double molWeight); double[] ConvertToBaseUnit(string dimensionName, string displayUnit, double valueInDisplayUnit);

All exposed via the DImensionTag. IT deals EXPLICITELY with those use cases

msevestre · 2021-03-25T20:06:48Z

R/utilities-dimensions.R

+  validateUnit(fromUnit, dimension)
+  validateUnit(toUnit, dimension)
+
+  dimensionTask <- getDimensionTask()


nope

toUnit <- function(quantityOrDimension, values, targetUnit, molWeight = NULL) {

Specifically the parameter is called quantityOrDimension.

None of the proposed changes regarding reimplementing conversion by hand belong in Core. Helper function to ensure that dimension and unit etc.. exists, yes. The rest probably not :)

codecov-io · 2021-03-25T20:17:32Z

Codecov Report

Merging #495 (2d6d243) into develop (4c3654b) will decrease coverage by 3.52%.
The diff coverage is 1.78%.

@@             Coverage Diff             @@
##           develop     #495      +/-   ##
===========================================
- Coverage    88.52%   85.00%   -3.53%     
===========================================
  Files           72       73       +1     
  Lines         1325     1380      +55     
===========================================
  Hits          1173     1173              
- Misses         152      207      +55

Impacted Files	Coverage Δ
R/messages.R	`100.00% <ø> (ø)`
R/utilities-dimensions.R	`0.00% <0.00%> (ø)`
R/utilities-units.R	`100.00% <100.00%> (ø)`

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 4c3654b...2d6d243. Read the comment docs.

PavelBal · 2021-03-26T12:43:35Z

R/init-package.R

@@ -18,4 +18,6 @@ initPackage <- function() {
  apiConfig$pkParametersFilePath <- filePathFor("OSPSuite.PKParameters.xml")

  rClr::clrCallStatic("OSPSuite.R.Api", "InitializeOnce", apiConfig$ref)
+


This allows to populate the enum after loading the package..

I did not like this because it was creating the variable int he PARENT environment which was global. Now what I changed is that I am initializing the variable in a method and so now the variable are not global anymore

PavelBal · 2021-03-26T12:48:22Z

R/utilities-units.R

+#' @param toUnit String name of the unit to convert to
+#' @param dimension Dimension of the units (conversion between mass and molar possible when
+#' \code{MW} is provided)
+#' @param MW Optional molecule weight in g/mol to use when converting, for example, from molar to mass amount or concentration


I think MW should be passed as g/mol, this is the only unit a user expects.
@msevestre

MW Optional molecule weight in g/mol

yes?

Ok I see your point. The problem is that we always pass parameters in core unit
height should be dm etc..
Introducing a change like this would not be logical, specifically because the weight generally comes from simulation$molWeightFor which returns in core unit.
I am still not 100% sure I understand why we need such a method. If this is just for the MW, then we could add another flag to toUnit (mw.in.base.unit = true) by default or sthg like this

No its not for the MW, its for conversion of values that are not in base units - and most probably these would be the values that do not come from a simulation (that can be converted with toUnit). It is for data that are defined in other units - loaded from excel, generated by hand, or whatsoever.. And in this case MW most definitely will be in g/mol (even in the data exporter we always assume MW is in g/mol).

I see your point that this will be inconsistent with the rest of methods. But I also do not see the use case for this function with MW being in the base unit.

LEt's discuss WEdnesday. I do not understand why a user would not convert an array using toUnit instead of dealing with some weird factors

PavelBal · 2021-03-26T12:48:52Z

R/utilities-units.R

+#' @include enum.R
+#'
+#' @export
+Dimensions <- list()


Initialize empty and then populate after package is loaded.

I would like not to rely on this too much for now because @abdullahhamadeh is going to implement a parsign method see #478

PavelBal · 2021-04-01T10:54:32Z

R/utilities-units.R

@@ -15,7 +76,7 @@
 #'
 #' valuesInBaseUnit <- toBaseUnit(par, c(1000, 2000, 3000), "ml")
 #' @export
-toBaseUnit <- function(quantityOrDimension, values, unit, molWeight = NULL) {
+toBaseUnit <- function(quantityOrDimension, values, unit, molWeight = NULL, molWeightUnit = NULL) {


To be consistent, both "toBaseUnit" and "toUnit" allow now molWeight in custom unit.

PavelBal · 2021-04-01T10:56:12Z

R/utilities-units.R

 #' @export
-toUnit <- function(quantityOrDimension, values, targetUnit, molWeight = NULL) {
+toUnit <- function(quantityOrDimension, values, targetUnit, sourceUnit = NULL, molWeight = NULL, molWeightUnit = NULL) {


This would be the new signature that allows to convert from non-base units.

PavelBal · 2021-04-01T10:56:52Z

R/utilities-units.R

@@ -74,8 +147,20 @@ toUnit <- function(quantityOrDimension, values, targetUnit, molWeight = NULL) {
  }

  if (is.null(molWeight)) {
+    #Convert values to base unit first if the source unit is provided
+    if (!is.null(sourceUnit)){


This is repeated in both if-cases, but I cannot think of a better solution now.

PavelBal · 2021-04-01T10:57:21Z

R/utilities-units.R

+#' enum of all units for each dimension
+#'
+#' @export
+DimensionsUnits <- list()


How about this naming for the enum with the units?

PavelBal · 2021-04-01T11:09:13Z

@msevestre I don't get it, why is the build failing? Obviously it somehow uses the old dlls, but I cannot figure out why. It works on my local machine.

msevestre · 2021-04-01T13:57:27Z

@PavelBal Ok I am going to check it out

Moved methods to utilities-units Using core methods

msevestre · 2021-04-02T16:49:26Z

NAMESPACE

@@ -77,6 +80,8 @@ export(loadAgingDataFromCSV)
 export(loadDataRepositoryFromPKML)
 export(loadPopulation)
 export(loadSimulation)
+export(ospDimensions)
+export(ospUnits)


@PavelBal This is how we decied to name those guys to avoid any conflicts and it's also easy to type to get intellisense

msevestre · 2021-04-02T16:50:18Z

R/init-package.R

@@ -18,4 +18,6 @@ initPackage <- function() {
  apiConfig$pkParametersFilePath <- filePathFor("OSPSuite.PKParameters.xml")

  rClr::clrCallStatic("OSPSuite.R.Api", "InitializeOnce", apiConfig$ref)
+


I did not like this because it was creating the variable int he PARENT environment which was global. Now what I changed is that I am initializing the variable in a method and so now the variable are not global anymore

msevestre · 2021-04-02T17:02:17Z

R/utilities-units.R

+  validateIsString(unit)
+  validateDimension(dimension)
+  dimensionTask <- getDimensionTask()
+  rClr::clrCall(dimensionTask, "HasUnit", enc2utf8(dimension), encodeUnit(unit))


this needs to be encodeUnit for unuit

msevestre · 2021-04-02T17:02:51Z

R/utilities-units.R

+
+initializeDimensionAndUnitLists <- function() {
+  #This initializes the two lists in the parent environment which is the package environments
+  ospDimensions <<- getDimensionsEnum()


@PavelBal Here is how we do not polute the global namespace (I think...)

PavelBal commented Mar 25, 2021

View reviewed changes

msevestre requested changes Mar 25, 2021

View reviewed changes

msevestre reviewed Mar 25, 2021

View reviewed changes

PavelBal commented Mar 26, 2021

View reviewed changes

PavelBal commented Apr 1, 2021

View reviewed changes

PavelBal and others added 13 commits April 2, 2021 12:10

Add utilities-dimensions.R

fdd4b39

Add utilities-dimensions.R

dfde902

merge develop..still code needs to be reviewed

f56f817

Some more updates

19d2ee1

Remove existsDimension from export

95811fb

Added tests

aed6ba2

Moved methods to utilities-units Using core methods

Styler

2fe8080

Remove obsolete error messages

983bb66

- Extend toUnit to convert from non-base unit

660e8fc

Add molWeightUnit to toBaseUnit

e088fa2

Styler

d4dfb13

Manual update

75fe7ee

Rebase on develop to avoid weird conflicts

5c9144d

msevestre force-pushed the utilities-dimensions branch from d4339a4 to 5c9144d Compare April 2, 2021 16:48

msevestre added 2 commits April 2, 2021 12:59

Rebase on develop to avoid weird conflicts

c281534

Rebase on develop to avoid weird conflicts

ba8aebb

msevestre reviewed Apr 2, 2021

View reviewed changes

msevestre added 2 commits April 2, 2021 13:22

Fixes issue with missing parameter

5ddb38f

Fixes weird tests failing

dba68d8

msevestre merged commit 7d8d39e into develop Apr 2, 2021

msevestre deleted the utilities-dimensions branch April 2, 2021 18:12

		@@ -18,4 +18,6 @@ initPackage <- function() {
		apiConfig$pkParametersFilePath <- filePathFor("OSPSuite.PKParameters.xml")

		rClr::clrCallStatic("OSPSuite.R.Api", "InitializeOnce", apiConfig$ref)

Utilities dimensions #495

Utilities dimensions #495

Conversation

PavelBal commented Mar 25, 2021

Choose a reason for hiding this comment

PavelBal commented Mar 25, 2021

PavelBal commented Mar 25, 2021

msevestre commented Mar 25, 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

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

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-io commented Mar 25, 2021

Codecov Report

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

Choose a reason for hiding this comment

PavelBal commented Apr 1, 2021

msevestre commented Apr 1, 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