-
-
Notifications
You must be signed in to change notification settings - Fork 74
Deprecate metric argument and property, rename to inv_metric #804
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
base: develop
Are you sure you want to change the base?
Conversation
…mer overloading of metric argument
249c9a6
to
4db7ecb
Compare
@mitzimorris I'd appreciate a review if you are able! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly: the metric is specified by the inverse mass matrix which is either unit, dense, or diagonal, therefore the only property name we change is "metric" to "inv_metric" which holds, directly or indirectly, a matrix or list of matrices?
Therefore we should say "inverse mass matrix" in the doc comments and messages when talking about the matrix and its contents.
Otherwise this looks good and I like the encapsulated functions for dealing with metric files.
cmdstanpy/model.py
Outdated
a valid filepath to a JSON or Rdump file which contains an entry | ||
'inv_metric' whose value is either the diagonal vector or | ||
the full covariance matrix. | ||
:param metric: Specify the type of the mass matrix. Options are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type of the inverse mass matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected!
The mass matrix and metric are the same matrix. For a multivariate normal, the best value is the covariance, as that preconditions the multivariate normal back to unit covariance. See page 31 of Betancourt's conceptual intro to HMC. For years, I'd had a bunch of this turned around in my head and in our doc and on the board as in the Stan meeting. Sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Submission Checklist
Summary
Closes #803
This renames the
metric
property ofCmdStanMCMC
toinv_metric
, with the old property still available but raising a warning.Secondly, it updates how the initial inverse metric is provided. The pre-existing
metric
argument is split into two,metric
which still exists to specify only the metric type (e.g."diag_e"
), and a newinv_metric
which specifies the value. This handling is written from-scratch using some of the helpers that already existed for e.g. init files. Besides the existing types, it can also now accept numpy arrays, meaning it is valid to writemodel.sample(..., inv_metric=old_fit.inv_metric)
I am not completely confident that the error handling is as robust as previous, it's probably possible to come up with a bad argument that wouldn't be caught until actually running cmdstan, rather than trapped as a nicer error in the Python level.
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):
Simons Foundation
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: