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

Add VI for linear regression #154

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add VI for linear regression #154

wants to merge 4 commits into from

Conversation

ShouvikGhosh2048
Copy link
Collaborator

No description provided.

@ShouvikGhosh2048
Copy link
Collaborator Author

ShouvikGhosh2048 commented Dec 6, 2024

This seems like a very good idea.

@sourish-cmi @Nandini-Jaiswal

I've added VI for linear regression.

  • I split BayesianRegression into BayesianRegressionMCMC and BayesianRegressionVI structs.
  • I pass a boolean use_vi to determine whether to use MCMC or VI.
  • For VI, I store the distribution, and sample it during prediction. (This technically makes the prediction random each time, I could change it to sample during creation and store the sampled values instead dist, if you prefer that.)
  • I made seperate structs for BayesianRegressionMCMC and BayesianRegressionVI to store the chain and dist respectively. However we could just convert both to a matrix of samples (we will sample the dist for VI) and just have a single BayesianRegression struct which stores the matrix of samples. We would lose the info about the chain and dist though, I'm not sure if its important.

Copy link
Collaborator

@sourish-cmi sourish-cmi left a comment

Choose a reason for hiding this comment

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

Looks like very good improvement.

@sourish-cmi
Copy link
Collaborator

@ShouvikGhosh2048 Can you please add tests and check why the doc test is failing? Do you want me to provide some test!

@ShouvikGhosh2048
Copy link
Collaborator Author

ShouvikGhosh2048 commented Dec 6, 2024

I've added tests for VI, those are passing (right now just whether they run, I'll add a test to make sure that the MCMC and VI predictions are close). The doc tests are failing since I haven't made the changes for VI, I'll make them once we decide on the API.

@sourish-cmi sourish-cmi linked an issue Dec 6, 2024 that may be closed by this pull request
@sourish-cmi
Copy link
Collaborator

Okay - can we meet over Google Meet?

@ShouvikGhosh2048
Copy link
Collaborator Author

I've made the changes we discussed and updated the docs.

I ended up removing use_vi and instead I use a BayesianAlgorithm struct defined here:
https://github.com/xKDR/CRRao.jl/pull/154/files#diff-b989ddbe26d5a6bf4e0714d4bc169310463a8e8bbcadaf6d5903d5fe34f6fc02R395-R432

I went with this since the algorithms have different parameters (MCMC needs the number of iterations and the number of initial observations we discard, and VI needs the number of samples, number of VI iterations and number of samples per VI step).

So now we do:
model = fit(@formula(MPG ~ HP + WT + Gear), mtcars, LinearRegression(), prior) - MCMC
model = fit(@formula(MPG ~ HP + WT + Gear), mtcars, LinearRegression(), prior, VI()) - VI

If the API is fine, this PR is ready to merge.

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.

Add Variational Inference
2 participants