Do not create new quantity objects if utilities-simulation-results if… #463
Conversation
… quantites are passed.
|
|
||
| if (length(paths) == 0) { | ||
| return(list(data = NULL, metaData = NULL)) | ||
| # If quantities are passed, get their paths. |
There was a problem hiding this comment.
Ensure that we have a list of paths and a list of corresponding quantities. If quantities are provided, get their paths, and vice versa.
| metaData[[path]] <- list(unit = quantity$unit, dimension = quantity$dimension) | ||
| values[[path]] <- simulationResults$getValuesByPath(path, individualIds, stopIfNotFound) | ||
| } | ||
| values <- lapply(paths, function(path){ |
There was a problem hiding this comment.
Better to use lapply instead of iterating.
There was a problem hiding this comment.
it it? you are lapply twice now instead of one loop.
There was a problem hiding this comment.
OK changed to iteration.
There was a problem hiding this comment.
@PavelBal I like the usage of lapply better (easier to read) But I am not sure that this it is all around better because of double iteration.
Would you mind putting it back in?
| metaData[[path]] <- list(unit = quantity$unit, dimension = quantity$dimension) | ||
| values[[path]] <- simulationResults$getValuesByPath(path, individualIds, stopIfNotFound) | ||
| } | ||
| values <- lapply(paths, function(path){ |
There was a problem hiding this comment.
it it? you are lapply twice now instead of one loop.
R/utilities-simulation-results.R
Outdated
| paths <- unlist( | ||
| lapply(quantities, function(x) x$path) | ||
| ) | ||
| names(quantities) <- paths |
There was a problem hiding this comment.
That can be moved outside of if then since you are doing it everywhere
R/utilities-simulation-results.R
Outdated
| # If quantities are passed, get their paths. | ||
| if (isOfType(quantitiesOrPaths, Quantity)) { | ||
| quantities <- uniqueEntities(quantitiesOrPaths) | ||
| paths <- unlist( |
There was a problem hiding this comment.
unless the formatter does it autimatically (!?!) put this in one line. (unlist(lapply())
There was a problem hiding this comment.
Re-run styler on the package.
|
A few minor comments. Looks good. |
- Run styler
R/entity.R
Outdated
| ), | ||
| active = list( | ||
| #' @field path The path of the entity in the container hiearchy without the simulation name. (read-only) | ||
| path = function(value) { | ||
| private$wrapExtensionMethod(EntityExtensions, "ConsolidatedPath", "path", value) | ||
| if (is.null(private$.path)) { |
There was a problem hiding this comment.
Cache the path as it never changes.
| finally = { | ||
| file.remove(pkParameterResultsFilePath) | ||
| } | ||
| dataFrame <- tryCatch( |
|
@msevestre I updated according to your comments. Use-case scenario: simulate a steady-state of a model when all quantities are provided. Version 9.2 needs 55 seconds, of which 17 are simulation time. With this PR, first run takes 33 seconds, the consequent runs only 20. The fist run takes longer because of caching of the unit, dimension, and path of each quantity. |
|
@PavelBal This is kind of crazy that it takes so much just to read from the .NET layer. I think this is a very good investigation brother and we should be careful in the future to cache such read only info as early as possible |
|
@PavelBal I'll merge this for now and we can revisit |
|
@PavelBal However the tests are failing. We lose the "Error when setting values" etc.. You need to throw an exception when value is provided |
| @@ -59,6 +59,17 @@ DotNetWrapper <- R6::R6Class( | |||
| } | |||
| }, | |||
|
|
|||
| wrapExtensionMethodCached = function(typename, methodName, propertyName, cachedValue, value) { | |||
There was a problem hiding this comment.
@msevestre look at this, I added a wrapper for cached fields.
|
@msevestre Please check the method for cached fields, otherwise we are good to go I think. |
|
@PavelBal I am wondering if we should nto cash the isTable stuff in the formula as well. This was also taking a lot of time and is till using extension methods |
|
I wasn't sure whether these fields are always constant or can change their values. E.g., if changing the value of a parameter to a constant... If you can make a list of those fields that are definitely immutable, caching them would be a good thing. |
|
There are using extension methods anyways which is readonly. So caching it would not change the behavior. Just a bit of overhead |
… quantites are passed.
See #462