search

google-deepmind / torch-distributions

Probability distributions, wrapped for Torch.
BSD 3-Clause "New" or "Revised" License
61 stars 25 forks source link

Chi2 test does not always return the chi2 value #20

Closed jucor closed 11 years ago

jucor commented 11 years ago

When a data point is out of support, the chi2 statistical test does not return the chi2 statistic value.

This pull-request fixes it and adds corresponding unit tests.

jucor commented 11 years ago

Merging -- @d11 , check it out, it's a nice example of unit tests that fail even though they have full coverage :-)

d11 commented 11 years ago

Aha! Good point. I can't think of a good way to catch this case automatically, off-hand… hmm.

On 28 Nov 2013, at 11:21, Julien Cornebise notifications@github.com wrote:

Merging -- @d11 , check it out, it's a nice example of unit tests that fail even though they have full coverage :-)

— Reply to this email directly or view it on GitHub.

jucor commented 11 years ago

Aha! Good point. I can't think of a good way to catch this case automatically, off-hand… hmm.

Neither can I, since it's a problem of the function not respecting the contract. Then again, the contract is very flexible, by design, in Lua. I'm not sure it's even "written" -- as opposed to C++, for example, which goes the opposite extreme.

d11 commented 11 years ago

Yes, I nearly ranted about Lua's multiple return semantics in my previous reply but then I decided that was a cop out. (I think Python's tuples are a better solution without being overly restrictive, to be honest.) A half-solution might be possible by parsing the return statement to count the number of available return values, but that still wouldn't account for variadic returns (e.g. unpack), where the number of values returned is only determined at runtime...

On 28 Nov 2013, at 11:29, Julien Cornebise notifications@github.com wrote:

Aha! Good point. I can't think of a good way to catch this case automatically, off-hand… hmm.

Neither can I, since it's a problem of the function not respecting the contract. Then again, the contract is very flexible, by design, in Lua. I'm not sure it's even "written" -- as opposed to C++, for example, which goes the opposite extreme. — Reply to this email directly or view it on GitHub.

jucor commented 11 years ago

Hahaha, for once I'm not the one ranting ;-) I think what we have now is a fair tradeoff, that'll do.

On 28 November 2013 11:34, Dan Horgan notifications@github.com wrote:

Yes, I nearly ranted about Lua's multiple return semantics in my previous reply but then I decided that was a cop out. (I think Python's tuples are a better solution without being overly restrictive, to be honest.) A half-solution might be possible by parsing the return statement to count the number of available return values, but that still wouldn't account for variadic returns (e.g. unpack), where the number of values returned is only determined at runtime...

On 28 Nov 2013, at 11:29, Julien Cornebise notifications@github.com wrote:

Aha! Good point. I can't think of a good way to catch this case automatically, off-hand… hmm.

Neither can I, since it's a problem of the function not respecting the contract. Then again, the contract is very flexible, by design, in Lua. I'm not sure it's even "written" -- as opposed to C++, for example, which goes the opposite extreme. — Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/jucor/torch-distributions/pull/20#issuecomment-29457690 .