search

google-deepmind / torch-distributions

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

Have mvn.rnd support *, 1xD, * calls as if they were *, D, * #16

Closed jucor closed 11 years ago

jucor commented 11 years ago

As I was adding tests where mu is 1xD (instead of NxD or D), torch has suddenly segfaulted on me -- both qth and th. @d11, can you please see if you have the same problem with your install? I'm at a loss to understand what's going on.

(edit: fixed, see below)

jucor commented 11 years ago

OK, I've avoided the segfault by passing v1:clone() to the tests. What triggered the segfault is that the result tensor got resized from NxD to 1xD during the course of the systematic tests. Of course, while this was indeed a bug, it should not make torch segfault, but it does. At least now that's fixed, we see the tests failing as they should.

jucor commented 11 years ago

Hi @d11 This is a big refactor of the arguments parsing in mvn.rnd. Could you please see what you think of it? Especially commit ad510ac, where there are 5 systematic tests whose former behaviour I do not understand: do I misunderstand something when I think they should error?

d11 commented 11 years ago

Hmm, yes, I was rather undecided on the behaviour in these cases. It comes from the logic that when it is possible to infer the desired output size from the parameters, we should always resize the given output tensor accordingly. It depends whether we want to just treat the output tensor as a block of memory to be written to or whether we want to enforce that the user has given it a sensible size. Since you found the current behaviour to be unexpected I think throwing an error in these cases is reasonable, and the potential for catching confusing bugs probably outweighs the slight inconvenience of having to resize the output tensor explicitly… Cheers Dan

On 22 Nov 2013, at 20:05, Julien Cornebise notifications@github.com wrote:

Hi @d11 This is a big refactor of the arguments parsing in mvn.rnd. Could you please see what you think of it? Especially commit ad510ac, where there are 5 systematic tests whose former behaviour I do not understand: do I misunderstand something when I think they should error?

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

d11 commented 11 years ago

Hmm, I'm trying your unit test, without the fix, but I've not actually been able to reproduce the segfault so far. Would be interested to try this when you're back! Perhaps it's torch-version-dependent. But the changes all seem good to me, anyhow. :)

jucor commented 11 years ago

Interesting ! I'm back today, we can compare the torch commit I've used to have the segfault. Or we can move on :)