dscr icon indicating copy to clipboard operation
dscr copied to clipboard

Added addScenarioGroup function

Open BenjaminPeter opened this issue 9 years ago • 6 comments

I wrote a function addressing Issue #9 for my own purposes, hopefully it is useful for others.

BenjaminPeter avatar May 20 '15 20:05 BenjaminPeter

@BenjaminPeter the build failed - any idea why?

stephens999 avatar May 20 '15 22:05 stephens999

@stephens999 not sure - the log indicates the build environment is missing some packages (neither of which I did anything with). Is it possible that some configurations changed?

* checking package dependencies ... ERROR
Packages required but not available: ‘dplyr’ ‘knitr’

In general, what is the preferred way to update the package? I just ran make check locally, which passed. Now realized that I probably should have ran roxygenize() beforehand, so now did that as well.

BenjaminPeter avatar May 21 '15 00:05 BenjaminPeter

@BenjaminPeter I was going to ask you to re-run roxygen2 to document your function, but you're one step ahead of me. :)

I think the build failure was just a sporadic CRAN hiccup.

ramanshah avatar May 21 '15 16:05 ramanshah

great - it passes now. I like the code, but have suggestions for simplification. Specifically I'm wondering whether it might be simpler (and more flexible) to have the addScenarioGroup function so that it takes just one dataframe of args, with each row being a combination of args and names can be a vector with same length as number of columns of args. (optionally names could be included in the dataframe? ) If some arguments are constant then this would be just a constant column and that is pretty easy for the user to specify. I think this code would simplify both the logic and the code. If necessary we could also provide helper functions to create dataframes containing all combinations of parameters and automatic names. What do you think @BenjaminPeter ?

stephens999 avatar May 21 '15 17:05 stephens999

@stephens999 Not sure if I am understanding you correctly; do you mean instead of having args and flexible.args as distinct arguments, just have args that is now supposed to be a data frame?

If so, I prefer the current design because I find it more clear because

  1. it is clear in the sense that the different functionality comes from arguments, as opposed to overloading args to mean different things depending on context,
  2. It is very easy to change from addScenario to addScenariogroup by just creating a data frame with the parameter to be changed, instead of having to redo the entire function. Constant args can be included in flexible.args without issues.
  3. It allows passing of things that don't fit into data frames (notably I pass a function in args for my application)

regarding names, see the recent pull-request.

BenjaminPeter avatar May 21 '15 18:05 BenjaminPeter

Sorry @BenjaminPeter I've been delinquent on merging this. Can you rewrite to respect snake case and I will merge...

stephens999 avatar Sep 02 '15 21:09 stephens999