-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cli commands in ospsuite r #1483
Conversation
- run simulations from snapshot files - convert between snapshot and project files + Add tests + Add test snapshot and project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works great! Code implementation looks very neat and clean
Many unnecessary files added (.pdb, ConsoleApp, etc.) |
R/snapshots.R
Outdated
@@ -0,0 +1,116 @@ | |||
#' Run Simulations From Snapshot Files | |||
#' | |||
#' @param ... character strings, path to snapshot files or a directory containing snapshot files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not introducing an explicit parameter instead of ...
, e.g.
runSimulationsFromSnapshot <- function(snaphotPathsOrFolder, output = ".", csv = TRUE, pkml = FALSE, xml = FALSE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this way we can pass several files or folder in one call:
runSimulationsFromSnapshot("snapshot.json","snapshotFolder", exportCSV = TRUE, exportPKML = TRUE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that:
runSimulationsFromSnapshot(c("snapshot.json","snapshotFolder"), exportCSV = TRUE, exportPKML = TRUE)
Will also work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that:
runSimulationsFromSnapshot(c("snapshot.json","snapshotFolder"), exportCSV = TRUE, exportPKML = TRUE)Will also work
that's what I had in mind when suggesting introducing a named argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With ...
, both work
+ prefix format with "export"
+ formating
#Conflicts: # DESCRIPTION # inst/lib/ConsoleApp.deps.json # inst/lib/ConsoleApp.dll # inst/lib/PKSim.Assets.Images.dll # inst/lib/PKSim.Assets.dll # inst/lib/PKSim.CLI.Core.dll # inst/lib/PKSim.Core.dll # inst/lib/PKSim.Infrastructure.dll # inst/lib/PKSim.Presentation.dll # inst/lib/PKSim.R.dll
Created on 2024-09-05 with reprex v2.1.0