forcedotcom / distributions

Low-level primitives for collapsed Gibbs sampling in python and C++
BSD 3-Clause "New" or "Revised" License
33 stars 25 forks source link

implement NormalInverseWishart component model #75

Closed stephentu closed 10 years ago

stephentu commented 10 years ago

Here's our implementation of NIW, put in the distributions framework.

Several high level notes:

fritzo commented 10 years ago

To use external code under another license, we have adopted the convention of keeping the code in a separate vendor/ directory, e.g. distributions/include/dustributions/vendor/fmath. Could.you copy matt Johnsons to a new file in a distributions/distributions/vendor/ directory, add his MIT license header (as we are legally obligated) and import them in your code?

stephentu commented 10 years ago

Ugh, would there be any objection to bumping the compiler requirement from g++-4.6 to g++-4.8? the headers for std::chi_squared_distribution are sort of broken in 4.6 (see https://travis-ci.org/forcedotcom/distributions/jobs/32043162)

fritzo commented 10 years ago

@cap what's your estimate on when we might move from Ubuntu 12.04 to 14.04 internally? I develop distributions+loom on both already but those are only two components.

stephentu commented 10 years ago

I can also hack around this by copying the implementation into vendors for the time being

fritzo commented 10 years ago

Wait, we already get around this by using the <double> specialization: https://github.com/forcedotcom/distributions/blob/master/include/distributions/random.hpp#L73

fritzo commented 10 years ago

Hey Stephen , this is awesome and I'm excited to start using NIW2 for (long,lat) data. I'm happy to merge as soon as:

  1. Matt's MIT license is added in vendor/
  2. tests pass
  3. advance vs add_group is resolved
  4. style conforms (braces on if/else/for and whitespace between operators)

Post-merge I'd like to speed up Scorer (caching the inverted matrix) and implement Mixture.

stephentu commented 10 years ago

As for Matt's MIT license, I copied the license text into stats.py, is that sufficient or do we need a separate LICENSE file somewhere?

stephentu commented 10 years ago

I'm having a hard time reproducing the errors on OS X, so give me some time while I spin up an ubuntu VM to try to match the travis environment more closely

stephentu commented 10 years ago

Ah interesting, in setup.py-- is there any reason why -ffast-math -funsafe-math-optimizations is only enabled for linux builds and not OS X? this might be causing some of the discrepancy

stephentu commented 10 years ago

ok the bug was:

const VectorXf &x = ... // bad
const VectorXf x = ... // good

I moved some stuff around, so you may want to take another look at the diff

fritzo commented 10 years ago

OK, looks great. Just a little cleanup before we merge. Then Jonathan or I can take a look at the gof testing.

fritzo commented 10 years ago

I'm going to try to integrate cpplint with our tests to avoid manually checking style...

fritzo commented 10 years ago

OK I just pushed cpplint integration. Apologies for slightly changing style, but I thing this gets rid of some of my own idiosyncrasies.

pip install -r requirements.txt    # or just pip install cpplint
make lint_cc                       # or just make