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

expose CmdStan methods optimize, variational, generate_quantities, sampling with fixed_param #58

Closed
mitzimorris opened this issue Jun 29, 2019 · 13 comments
Labels
feature New feature or request

Comments

@mitzimorris
Copy link
Member

Summary:

The beta version of CmdStanPy only has function sample which runs the NUTS-HMC sampler. Expose the following additional CmdStan methods:

  • optimize
  • variational
  • generate_quantities
  • sampler algorithm fixed_param which is used for Stan programs without any parameters.

This is an umbrella issue. We need per-method issue to break this down into managable chunks. This issue is intended to be used to work out the best possible factorization of the CmdStanPy code base, as well as the best function and argument names for cross-interface compatibility.

Description:

The CmdStan CLI has common arguments and method and algorithm-specific arguments.
As currently implemented, CmdStanPy has a helper class SamplerArgs which is a composite of the data, output, and NUTS sampling algorithm arguments. Keep as a single composite, or refactor into sub-compontents?

Each method writes its output to the specified output file in a method-specific format - this requires writing additional parsing routines, and adding methods to the StanFit (fka RunSet) object to return this information using the appropriate Python object.

Additional Information:

The CmdStan argument names don't line up with RStan/PyStan names:

  • sample -> sampling
  • optimize -> optimizing
  • variational -> vb

currently only CmdStan exposes the method stan::services::generate_quantities. should this be called generated_quantities? This seems like the better name, in which case, we should change the CmdStan CLI arguments as well.

Current Version:

@yamamotoseiji
Copy link

Thanks for writing up this issue @mitzimorris ! We have three engineers who can potentially help get cmdstanpy over the finish line, depending on whether we choose to focus on cmdstanpy or pycmdstan. Are all four methods here needed by Prophet, or just the optimize method? Also, could you elaborate more on the nature of the affiliation between this package and Stan itself? It seems like PyStan and cmdstanpy are somehow related to Stan, but pycmdstan is not. Is it just about getting listed on the Stan websites or something else? Thanks!

@maedoc
Copy link
Contributor

maedoc commented Jun 30, 2019

It seems like PyStan and cmdstanpy are somehow related to Stan, but pycmdstan is not.

@yamamotoseiji please see the clarification I provided on the prophet issue; you will find pycmdstan in the git log of cmdstanpy, but cmdstanpy is the official Stan project.

@yamamotoseiji
Copy link

Thank you for the clarification @maedoc. Based on your comments on the prophet issue, it sounds like there isn't a plan to continue developing pycmdstan (which was the seed/inspiration for cmdstanpy) due to other important projects you are tracking and instead we should continuing developing pycmdstan because it will be the community-backed repo. With this, I think @wsuchy can go ahead and get started on the optimize method for cmdtanpy.

@mitzimorris
Copy link
Member Author

@yamamotoseiji many thanks! help from your @wsuchy and other members of your team would be much appreciated! I will file an issue for the optimize method.

a good place to start would be to take a look at what's on branch https://github.com/stan-dev/cmdstanpy/tree/feature/57-api-refactor - if he's available to do code review when this is ready for it, that would be awesome.

@mitzimorris
Copy link
Member Author

filed #60

@wsuchy
Copy link
Contributor

wsuchy commented Jul 1, 2019

Hi @mitzimorris!
We've noticed that the branch you've shared is still under construction (e.g. tests haven't been updated yet). In order to avoid merge / incompatibility conflicts caused by incoming changes, what would you think if we wait until the api changes are done?
Alternatively we could provide our changes (see example here: https://github.com/stan-dev/cmdstanpy/compare/master...wsuchy:addedOptimizing?expand=1 ) for refactoring API that would naturally emerge by implementing optimize call support.

@mitzimorris
Copy link
Member Author

your changes look good so far. I'd be happy to incorporate them into the refactor and then you could review - would that be OK?

I'm in the process of updating the unit tests right now.

@mitzimorris
Copy link
Member Author

@wsuchy - unit tests updated and passing! if you're available to do code review on this branch, I will file a PR.

I'd like to proceed stepwise - next step would be incorporating optimize - would this be OK with you?

@wsuchy
Copy link
Contributor

wsuchy commented Jul 2, 2019

@mitzimorris Yes, we will wait then for the PR.

@mitzimorris
Copy link
Member Author

@wsuchy - just filed a PR and invited you to join team!

This was referenced Jul 3, 2019
@mitzimorris
Copy link
Member Author

optimization added with PR #69

@mitzimorris
Copy link
Member Author

generated quantities added with PR #86

@mitzimorris
Copy link
Member Author

fixed_params added with PR #119, variational added with PR #120

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

No branches or pull requests

4 participants