Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using low-level rCrl methods in getOutputValues #518

Merged
merged 17 commits into from
May 20, 2021

Conversation

PavelBal
Copy link
Member

No description provided.

@PavelBal PavelBal requested a review from msevestre May 11, 2021 22:38
@PavelBal
Copy link
Member Author

See #519

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #518 (d0548e3) into develop (79bc93d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #518      +/-   ##
===========================================
+ Coverage    88.46%   88.53%   +0.06%     
===========================================
  Files           72       72              
  Lines         1422     1439      +17     
===========================================
+ Hits          1258     1274      +16     
- Misses         164      165       +1     
Impacted Files Coverage Δ
R/utilities.R 100.00% <ø> (ø)
R/error-checks.R 93.10% <100.00%> (+0.37%) ⬆️
R/messages.R 100.00% <100.00%> (ø)
R/utilities-output-selections.R 100.00% <100.00%> (ø)
R/utilities-parameter.R 100.00% <100.00%> (ø)
R/utilities-quantity.R 100.00% <100.00%> (ø)
R/utilities-simulation-results.R 98.24% <100.00%> (+0.20%) ⬆️
R/output-selections.R 37.50% <0.00%> (-25.00%) ⬇️
R/simulation-results.R 68.57% <0.00%> (+2.85%) ⬆️

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 79bc93d...d0548e3. Read the comment docs.

@PavelBal
Copy link
Member Author

@msevestre You can review now. I am using the "fromPath" methods implemented in Core.

@PavelBal
Copy link
Member Author

You have to teach me how to correctly update package versions...

R/utilities-simulation-results.R Outdated Show resolved Hide resolved
@msevestre
Copy link
Member

Let me do this for you:)

@PavelBal
Copy link
Member Author

Let me do this for you:)

Better so :D

R/utilities-output-selections.R Outdated Show resolved Hide resolved
#'
#' setParameterValuesByPath(list("Organism|Liver|Volume", "Organism|Liver|A"), c(2, 3), sim)
#' @export
setQuantityValuesByPath <- function(quantityPaths, values, simulation) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@msevestre Implementation using the .NET method for setting values by path

validateIsOfType(simulation, Simulation)
parameters <- sapply(parameterPaths, function(p) getParameter(p, simulation))
setParameterValues(parameters, values)
setQuantityValuesByPath(quantityPaths = parameterPaths, values = values,
Copy link
Member Author

Choose a reason for hiding this comment

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

setParameterValuesByPath can simply use setQuantityValuesByPath

@PavelBal
Copy link
Member Author

Open questions:

  1. withMetaData argument in getOutputValues
  2. addOutputs - argument returnQuantities introduced - discard? Do not return quantities at all (would require a not in release notes)
  3. New function setQuantityValuesByPath

PavelBal added 2 commits May 19, 2021 19:55
- Argument 'withMetaData' in 'getOutputValues' renamed to 'addMetaData'
R/messages.R Outdated Show resolved Hide resolved
R/messages.R Outdated Show resolved Hide resolved
R/utilities-parameter.R Outdated Show resolved Hide resolved
R/utilities-quantity.R Show resolved Hide resolved
R/utilities-simulation-results.R Outdated Show resolved Hide resolved
@msevestre
Copy link
Member

@PavelBal I updated to use single version of the methods . IsExplicitFormulaFromPath is never used anywhere so I am not sure where this is going to break

@PavelBal
Copy link
Member Author

@msevestre OK I think we are good to merge.

@msevestre msevestre merged commit 4daa4c4 into develop May 20, 2021
@msevestre msevestre deleted the performance-optimizations branch May 20, 2021 15:29
@msevestre
Copy link
Member

@PavelBal Be mindful of method rename and behavior. IsExplicitFormulasFromPath => IsExplicitFormulaByPath

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants