stan-dev / stan

Stan development repository. The master branch contains the current release. The develop branch contains the latest stable development. See the Developer Process Wiki for details.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
2.6k stars 370 forks source link

Adds option to turn off psis resampling and lp calculation for pathfinder #3249

Closed SteveBronder closed 10 months ago

SteveBronder commented 10 months ago

Submission Checklist

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 a lp__ column at least exists so here I just set the column of lp values to NaN. 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 to NaN.

Intended Effect

How to Verify

Tests were added for the new options and can be run with

python3 ./runTests.py ./src/test/unit/services/pathfinder/normal_glm_test.cpp

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:

WardBrian commented 10 months ago

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 const Eigen::Ref<const Eigen::Matrix<double, -1, -1>>& replacing the previous MatrixXd overload. Am I missing something?

SteveBronder commented 10 months ago

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

WardBrian commented 10 months ago

Passing a RowVector to a MatrixXd is free, right?

SteveBronder commented 10 months ago

Printing row vs. column vectors is the issue. I think we just need to change the CommaFormat

WardBrian commented 10 months ago

The writer class is pretty explicit that The input is expected to have parameters in the rows and samples in the columns. So imo, if there is anything which was passing a wrongly-shaped vector, we should fix that call site

SteveBronder commented 10 months ago

That's fine I removed that logic. Should be good to go now