search

pyro-ppl / brmp

Bayesian Regression Models in Pyro
Apache License 2.0
69 stars 8 forks source link

calculate scalar parameter map once on init to avoid rebuilding multiple times #83

Closed matteby closed 3 years ago

matteby commented 3 years ago

This PR addresses a TODO comment in the marginals function where the scalar parameter map is rebuilt on each call to get_scalar_param.

I've tested this with a model that has around 40000 parameters and it takes the run time of marginals from around 7 hrs. on a Mac Book Pro to around 220 seconds. I've also verified that the existing pytest tests work as before.

This is my first contribution to this project so please let me know if there are any guidelines or other things I need to do for a PR.

eb8680 commented 3 years ago

@matteby thanks for the PR! Looks good to me. Out of curiosity, can you say anything about what you're using brmp for and how extensively? As you may have noticed, we haven't had the developer bandwidth to continue adding new features, although what's here is fairly stable, but if people are actively using it we may need to reprioritize.

@neerajprad any idea why Travis isn't running on this PR? It looks like we have a .travis.yml file set up.

matteby commented 3 years ago

@eb8680 We're using BRMP for building random effects models on healthcare datasets. We have national level US datasets that we're modeling at zip code and census tract levels of granularity.

This is our first model built with BRMP so we haven't used it extensively but are evaluating how we go forward. Given the large number of parameters using numpryo for HMC is attractive. We've been debating whether we'll eventually need to take the BRMP generated code and use numpyro directly. From what I can tell there really isn't anything else in the python ecosystem comparable so I'd definitely love to see it continue.

I should add on the tests... one of the tests was failing for me but it was that way before my changes:

FAILED tests/test_brm.py::test_marginals_fitted_smoke[y ~ 1 + a + x + (1 | b)-non_real_cols0-family0-contrasts0-<lambda>2] - RuntimeError: Unable to handle autograd's threading in combination with .
null-a commented 3 years ago

From what I can tell there really isn't anything else in the python ecosystem comparable so I'd definitely love to see it continue.

@matteby: Looking through my notes just now reminded me of the bambi project. I thought it might be of interest if you've not seen it before.

matteby commented 3 years ago

From what I can tell there really isn't anything else in the python ecosystem comparable so I'd definitely love to see it continue.

@matteby: Looking through my notes just now reminded me of the bambi project. I thought it might be of interest if you've not seen it before.

@null-a Apologies, yes I forgot I actually did look at that a few months back. One reason we went with BRMP is HMC on numpyro.