search

brendanhasz / probflow

A Python package for building Bayesian models with TensorFlow or PyTorch
http://probflow.readthedocs.io
MIT License
171 stars 17 forks source link

Fix rand_rademacher for pytorch backend #60

Closed srwi closed 3 years ago

srwi commented 3 years ago

Hi, I noticed there was a problem where the rademacher distribution was implemented incorrectly for the pytorch backend. It was always returning -1. It now generates -1 and 1 correctly just like for the tensorflow backend.

Edit: test_rand_rademacher was always passing because it always generated -1. It is not immediately obvious to me why other seemingly unreleated tests are failing now. I might look into it again when I get the time.

brendanhasz commented 3 years ago

Hi! Good catch - thanks for finding the issue and making a PR!

Yeah looks like those failing tests are totally unrelated to your PR (just looks like some bounds/shape changes in the most recent version of PyTorch). I'm working on a separate PR to fix em (https://github.com/brendanhasz/probflow/pull/61), so I'll post back here once I merge that, and then after that if you don't mind merging master back into your fork the tests should pass, and then we can merge your PR. I probably won't get around to finishing the fixes for a day or two though.

Thanks again for the PR! 😄

srwi commented 3 years ago

Sounds good! No rush though. I'll merge your changes whenever you're ready. Thanks for your work!

brendanhasz commented 3 years ago

Hey @stnkl, I fixed the test issues in https://github.com/brendanhasz/probflow/pull/61 - if you don't mind merging master into your fork then the tests should pass, and we can merge your fix in. Thanks!

srwi commented 3 years ago

Thanks! I did a rebase and the tests passed now. 😊

brendanhasz commented 3 years ago

Awesome - thanks again so much for finding that bug and for your PR!