Skip to content

Added addScenarioGroup function #45

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

BenjaminPeter
Copy link

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

@stephens999
Copy link
Owner

@BenjaminPeter the build failed - any idea why?

@BenjaminPeter
Copy link
Author

@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.

@ramanshah
Copy link
Contributor

@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.

@stephens999
Copy link
Owner

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 ?

@BenjaminPeter
Copy link
Author

@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.

@stephens999
Copy link
Owner

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

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