Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove the intercept assumption in Gibbs samplers #23

Merged
merged 1 commit into from
Apr 9, 2022
Merged

Conversation

rlouf
Copy link
Member

@rlouf rlouf commented Apr 6, 2022

The samplers currently assume that there is an intercept term in the
regressions, and this term is treated separately from the other
regression parameters (Makalic & Schmidt 2016). This is limiting for two
reasons:

  1. We don't always want to run regressions with an intercept parameter;
    at the very least we'd like to control its prior value.
  2. This complicates the work for Standardize the sampler-constructor interface #16.

In this commit we generalize the Gibbs samplers by removing
the separation between the intercept beta0 and the other regression parameters.

Closes #17

@brandonwillard Many references mention having a separate prior for the intercept parameter, which does make sense. Are there cases that you know of where adding a shrinkage prior to the intercept changes the results?

Also I've noticed we don't have any tests to check that the samplers sample from the correct posterior distribution. We may want to add this in the near future.

The samplers currently assume that there is an intercept term in the
regressions, and this term is treated separately from the other
regression parameters (Makalic & Schmidt 2016). This is limiting for two
reasons:

1. We don't always want to run regressions with an intercept parameter;
at the very least we'd like to control its prior value.
2. This complicates the work for #16.

As a result in this commit we generalize the Gibbs samplers by removing
the separation between the intercept beta0 and the other regression parameters.
@rlouf rlouf requested review from zoj613 and brandonwillard April 6, 2022 15:10
@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #23 (c2f1748) into main (2dfa81f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          128       118   -10     
  Branches        10        10           
=========================================
- Hits           128       118   -10     
Impacted Files Coverage Δ
aemcmc/gibbs.py 100.00% <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 2dfa81f...c2f1748. Read the comment docs.

@rlouf
Copy link
Member Author

rlouf commented Apr 7, 2022

In this paper the authors have this unsubstantiated claim:

Unlike in the case of linear regression, the intercept parameter [of the logistic regression] must now be explicitly modelled. Special care should be taken to ensure that the intercept parameter is not penalized (e.g., by using a uniform prior distribution

@brandonwillard Since you've worked with these shrinkage priors before, I was wondering if you knew why they would say that / if we can safely ignore it?

@zoj613
Copy link
Member

zoj613 commented Apr 7, 2022

I would be nice to see the summary of the posterior samples using both approaches when we have data generated with an intercept term. I'm curious about how much the resulting parameter sample means would differ.

@rlouf rlouf self-assigned this Apr 8, 2022
@brandonwillard brandonwillard added the refactoring A change that improves the codebase but doesn't necessarily introduce a new feature label Apr 9, 2022
@rlouf rlouf merged commit 76131af into main Apr 9, 2022
@rlouf rlouf deleted the generalize-sampler branch April 9, 2022 04:59
@brandonwillard
Copy link
Member

brandonwillard commented Apr 9, 2022

@brandonwillard Many references mention having a separate prior for the intercept parameter, which does make sense. Are there cases that you know of where adding a shrinkage prior to the intercept changes the results?

It depends on the model and what you're trying to get from it. Applying shrinkage to the intercept should definitely change the results, unless the other prior one chooses for the intercept is equivalent in terms of whichever measure one's using to observe change.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
refactoring A change that improves the codebase but doesn't necessarily introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize horseshoe_nbinom_gibbs
3 participants