janezd / baycomp

MIT License
69 stars 15 forks source link

Fix random_state in hierarchical and sign test #10

Closed dpaetzel closed 2 years ago

dpaetzel commented 2 years ago

It seems as if #9 broke the hierarchical model which somehow doesn't get caught by the tests.

E.g. when doing

import numpy as np
import baycomp

runs = 10
cv = 10
a = np.random.random((runs, cv))
b = np.random.random((runs, cv))

probs = baycomp.two_on_multiple(a, b)
print(probs)

I get a

Traceback (most recent call last):
  File "/home/david/Code/baycomp/test.py", line 11, in <module>
    probs = baycomp.two_on_multiple(a, b)
  File "/home/david/Code/baycomp/baycomp/multiple.py", line 490, in two_on_multiple
    return call_shortcut(test, x, y, rope, names=names, plot=plot, **kwargs)
  File "/home/david/Code/baycomp/baycomp/utils.py", line 20, in call_shortcut
    sample = test(x, y, rope, *args, **kwargs)
  File "/home/david/Code/baycomp/baycomp/multiple.py", line 153, in __new__
    return Posterior(cls.sample(x, y, rope, nsamples=nsamples, random_state=random_state, **kwargs))
TypeError: sample() got an unexpected keyword argument 'random_state'

In this case, sample = test(x, y, rope, *args, **kwargs) corresponds to sample = HierarchicalTest(x, y, rope, *args, **kwargs) which means that HierarchicalTest.__new__ (which is inherited from Test) gets called. This results in Posterior(cls.sample(x, y, rope, nsamples=nsamples, random_state=random_state, **kwargs)) where cls is HierarchicalTest.

However, HierarchicalTest.sample doesn't yet have a random_state parameter (HierarchicalTest still uses np.random.*).

This PR fixes that by adding support for random_state to HierarchicalModel.sample as well as SignTest.sample.

janezd commented 2 years ago

Thanks! I apologize for the wait - I'm too busy...

dpaetzel commented 2 years ago

Thank you for your work on this and this library! :slightly_smiling_face: