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

cmdstanpy cmdstanr name consistency for sampler #221

Closed
mitzimorris opened this issue Mar 3, 2020 · 12 comments
Closed

cmdstanpy cmdstanr name consistency for sampler #221

mitzimorris opened this issue Mar 3, 2020 · 12 comments
Assignees

Comments

@mitzimorris
Copy link
Member

mitzimorris commented Mar 3, 2020

Summary:

improve consistency of argument names between CmdStanPy and CmdStanR

Description:

different names to sampler function

  • "cores" vs "num_cores"
  • "chains" vs "num_chains"
  • "warmup_iters" vs "num_warmup"
  • "sampling_iters" vs "num_sampling"
  • "step_size" vs. "stepsize"
  • "max_treedepth" vs "max_depth"

after discussion here: stan-dev/design-docs#15, argument names for sampler methods are:

  • cores (current name)
  • chains (current name)
  • iter_warmup instead of "warmup_iters"
  • iter_sampling instead of "sampling_iters"

Additional Information:

Provide any additional information here.

Current Version:

@mitzimorris
Copy link
Member Author

@jgabry - willing to go with CmdStanR versions - comments?

@bob-carpenter
Copy link
Contributor

Are both going to be consistent with CmdStan itself?

The only reason someone's suggesting num_warmup is that warmup is ambiguous. I prefer warmup_iter, because it disambiguates more cleanly than num_warmup. In general, num_ is redundant, as evidenced by nobody suggesting num_max_treedepth just because the value is a counting number.

step_size is the version consistent with separating words with underscores.

@jgabry
Copy link
Member

jgabry commented Mar 3, 2020

The reason we've been using num_samples, num_warmup, max_depth, and stepsize in CmdStanR is to match CmdStan. I don't love the CmdStan names but until they are changed (which I would support) I figured we should use them in the wrappers to avoid confusion.

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Mar 3, 2020

The reason we've been using num_samples, num_warmup, max_depth, and stepsize in CmdStanR is to match CmdStan.

@jgabry or @mitzimorris : can one of you clarify the issue to reflect this decision? The current text implies there'a a decision to make.

Just keep in mind that whatever gets decided here is probably going to have to carry over to a simplified CmdStan3. My guess is that most Stan users migrating to CmdStanR or CmdStanPy will be coming from RStan or PyStan.

Anyway, not a big deal. I don't want to derail development with a long digression on naming.

@jgabry
Copy link
Member

jgabry commented Mar 3, 2020

Basically the decision is when naming arguments should we:

a) Mirror current cmdstan names when possible
b) Mirror rstan/pystan ñames when possible
c) Use whatever names we think are best going forward

@jgabry
Copy link
Member

jgabry commented Mar 3, 2020

I’ve been doing (a) because that seemed simplest/cleanest until there is CmdStan3. When that happens we can then do whatever CmdStan3 does.

@bob-carpenter
Copy link
Contributor

I'd strongly prefer whatever decision is made now for CmdStanPy and CmdStanR to be carried forward for CmdStan3. I think that's more important than the decision itself.

@jgabry
Copy link
Member

jgabry commented Mar 3, 2020

In that case we should do (c), since I don't think either (a) or (b) will result in the most sensible names for CmdStan3 to use. @mitzimorris I think that means we should have another meeting and get on the same page for all the names

@mitzimorris
Copy link
Member Author

this issue was filed by mistake - after some discussion with folks in the office and checking against RStan and CmdStan, I filed the 2nd issue on CmdStanR, because I think that "chains" and "cores" is better.

@jgabry
Copy link
Member

jgabry commented Mar 3, 2020

What about the other arguments like num_warmup and num_samples? CmdStanR is mirroring the current CmdStan, but we could think ahead for what's best for CmdStan3 like @bob-carpenter was saying.

@mitzimorris
Copy link
Member Author

mitzimorris commented Mar 3, 2020

I started off thinking c) was the best option, hence 'warmup_iters', 'sampling_iters', and 'step_size'. but then folks raised objections about confusion coming from existing interfaces and all of the teaching materials and stack overflow, forums, etc discussions which are what is most likely to be pulled back by a search engine.

I'd like to close this issue - the question is where to continue this discussion?

keeping issue open - changed title and description

@mitzimorris mitzimorris reopened this Mar 4, 2020
@mitzimorris mitzimorris changed the title match CmdStanR arg names "chains" should be "num_chains", "cores" should be "num_cores" cmdstanpy cmdstanr name consistency for sampler Mar 4, 2020
@mitzimorris mitzimorris self-assigned this Mar 7, 2020
@mitzimorris
Copy link
Member Author

#222

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

No branches or pull requests

3 participants