-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Adds option to turn off psis resampling and lp calculation for pathfinder #3249
Conversation
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.
A few comments. Boolean flags generally make me nervous, especially when they are interdependent or change the sizes of the return values (and these do both!) but I don't see any other reasonable way of fulfilling this request
I still think adding more overloads to writer (e.g. for Block) is a very heavy handed move. As far as I can tell, things would work fine if there was a single version that took |
Let me try that. I think we will still need the vector and row vector versions but that might work nicely for the block overloads |
Passing a RowVector to a MatrixXd is free, right? |
Printing row vs. column vectors is the issue. I think we just need to change the CommaFormat |
The writer class is pretty explicit that |
That's fine I removed that logic. Should be good to go now |
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.
Latest push has a syntax error and I think some test code can be cleaned up, but otherwise these changes look good
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Adds the request from @avehtari in #3242 to add an option for turning off resampling and calculating the lp.
If
psis_resample
is false then psis resampling is not performed and the full set of samples is returned from all the individual pathfinders.If
return_lp
is false then the lp values are not calculated for each sample. I think a lot of upstream packages assume that alp__
column at least exists so here I just set the column of lp values toNaN
. This also means psis resampling is turned off since we cannot calculate the lp ratio and the full set of samples is returned from all the individual pathfinders.Something questionable here is that since we have to calculate the lp values for the set of samples we compute the elbo metric on we just get those lp calculations for free. So here I still include those lp values even if
return_lp
is false. But that could be weird for users. I documented it, but another approach here would be to just explicitly set all lp values in that column toNaN
.Intended Effect
How to Verify
Tests were added for the new options and can be run with
Side Effects
Documentation
Once we open the cmdstan PR we should add documentation for these new options
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: