search

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

fix bug in NIW templated protobuf_load()/protobuf_store() #77

Closed stephentu closed 10 years ago

stephentu commented 10 years ago

The C++ templated versions of protobuf_load()/protobuf_store() was buggy for any dimension > 2.

This wasn't caught with test cases, since the lp cython wrappers reimplement protobuf_load()/protobuf_store() without using the C++ versions (meaning there was no test coverage of these functions). As a result, I also added a C++ level test in src/test_protobuf.cc which simply calls protobuf_load()/protobuf_store() on the C++ objects. Consequently, this also found a sign compare warning in DirichletDiscrete's implementation.

Unfortunately, the tests currently don't do any smart equality checking-- we could compare the bytes of the protobuf messages, but floating point imprecisions might screw that up. This is strictly better than what we had before, though.

stephentu commented 10 years ago

If you have a better place to put these tests, or you have an idea on how we can implement almost equality checking w/o manually implementing it for each shared/group type, let me know. One way may be to get a handle on the protobuf reflection objects, and use that to intelligently compare the messages.

fritzo commented 10 years ago

Thanks for adding the tests. The protobuf C++ assert_close issue recently came up in Loom, and I implemented an ad-hoc macro LOOM_ASSERT_CLOSE deferring to a template function are_close in https://github.com/priorknowledge/loom/blob/master/src/assert_close.hpp Would it be useful to you for that to be moved upstream to distributions? If so, feel free to just copy the code in, and we'll start using the distributions version in Loom.