Skip to content

Some fixes/additions to the variational arguments#283

Merged
mitzimorris merged 11 commits intostan-dev:developfrom
b-r-oleary:bro-fix-variational-args
Aug 27, 2020
Merged

Some fixes/additions to the variational arguments#283
mitzimorris merged 11 commits intostan-dev:developfrom
b-r-oleary:bro-fix-variational-args

Conversation

@b-r-oleary
Copy link
Copy Markdown
Contributor

Submission Checklist

  • Run unit tests
  • Declare copyright holder and open-source license: see below

Summary

Hi! I was exploring the possibility of migrating a project from pystan to cmdstanpy in order to access a more up-to-date version of stanc via a python interface. During that exploration, I found that there was some unexpected behavior when working with the variational interface. I poked around in the codebase and found a few problems that I attempted to fix in this PR. In particular:

  • Some of the Real parameters in VariationalArgs were being constrained to be >=1 rather than >0, which resulted in unexpected validation errors when I supplied values less than 1, e.g. eta = 0.1, or tol_rel_obj = 0.05.
  • The interface did not support disabling eta adaptation, which is supported by cmdstan, and is supported by other stan interfaces.
  • When disabling eta adaptation, the output file parser was expecting output data provided by eta adaptation that isn't present, and was also expecting a particular value of eta=1.
  • cmdstanpy throws an error if stan reports that "The algorithm may not have converged.". Since pystan doesn't throw an error under these conditions, and since cmdstan returns a 0 exit status under this condition, this behavior was unexpected to me, especially given that I am sometimes interested in the output of variational inference even if the algorithm has not converged. So, I added an argument require_converged that would allow us to disable that check, without changing the default behavior of the function.

I ran the unit-tests locally in a container built from the Dockerfile in this repo and some of the tests failed, but I don't think that those test failures are related to any of these changes.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

Brendon O'Leary

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Copy link
Copy Markdown
Member

@mitzimorris mitzimorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many thanks for the additions and corrections - the VB changes are all good.
please revert changes to checks onstep_size and init_alpha, noted below.
if you run all the code through the black formatter, it should pass the flake8 tests.

Comment thread cmdstanpy/cmdstan_args.py
Comment thread cmdstanpy/cmdstan_args.py
@mitzimorris
Copy link
Copy Markdown
Member

thanks @ahartikainen for pointing out my mistake - the code is correct, it just needs to pass the linter and unit tests.

@b-r-oleary b-r-oleary force-pushed the bro-fix-variational-args branch from 7017b8f to 1950b27 Compare August 27, 2020 13:35
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #283 into develop will increase coverage by 0.21%.
The diff coverage is 95.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #283      +/-   ##
===========================================
+ Coverage    76.07%   76.28%   +0.21%     
===========================================
  Files            9        9              
  Lines         2194     2197       +3     
===========================================
+ Hits          1669     1676       +7     
+ Misses         525      521       -4     
Impacted Files Coverage Δ
cmdstanpy/utils.py 77.52% <85.71%> (+0.14%) ⬆️
cmdstanpy/cmdstan_args.py 91.66% <100.00%> (+0.87%) ⬆️
cmdstanpy/model.py 81.69% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baad6ce...1950b27. Read the comment docs.

@mitzimorris mitzimorris merged commit 318e6e0 into stan-dev:develop Aug 27, 2020
@mitzimorris
Copy link
Copy Markdown
Member

many thanks!

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.

4 participants