search

google-research / torchsde

Differentiable SDE solvers with GPU support and efficient sensitivity analysis.
Apache License 2.0
1.52k stars 195 forks source link

BrownianInterval tests + bugfixes #28

Closed patrick-kidger closed 3 years ago

patrick-kidger commented 3 years ago

Many things on BrownianInterval:

Fixed behaviour for calling BrownianInterval and BrownianTree for t outside of their range (it now clamps and raises a warning) (#9, #10)

Fixed bug that prevented ForwardSDE.g_prod from being called. Tided it slightly.

Fixed SDE solvers stepping outside the region of integration.

Btw @lxuechen I don't actually follow what you were saying about there being a bug in ReverseBrownian?

patrick-kidger commented 3 years ago

Oh also - I'd like to remove the .to method from BrownianInterval. (And perhaps for consistency from the others too.) Is this something we need to support for any reason?

The reason for this is that it turns out that torch.randn produces different results on different devices, and for different dtypes [but only provided the shape is large enough, strangely], so the reconstruction procedure used in BrownianInterval won't work, unless we always create in float64 on the CPU and copy it across, which I think would be unacceptably slow.

lxuechen commented 3 years ago

Thanks for the tests and bug fix. The K-S tests fails on my machine. I think it might be worthwhile to think about the pool_size argument for SeedSequence. Using a larger pool_size typically helps in normality, but could also drag down the speed.

lxuechen commented 3 years ago

Oh also - I'd like to remove the .to method from BrownianInterval. (And perhaps for consistency from the others too.) Is this something we need to support for any reason?

The reason for this is that it turns out that torch.randn produces different results on different devices, and for different dtypes [but only provided the shape is large enough, strangely], so the reconstruction procedure used in BrownianInterval won't work, unless we always create in float64 on the CPU and copy it across, which I think would be unacceptably slow.

Let's remove to for BrownianInterval only for now. Thanks for mentioning this.

lxuechen commented 3 years ago

I don't actually follow what you were saying about there being a bug in ReverseBrownian?

I fixed this on dev-strat-adjoint and added a comment; see this. Ultimately, it also has to do with how the adjoint functions are negated. I'm inclined to keep the current form for the adjoints.

lxuechen commented 3 years ago

As a minor side note, could we give the branches more meaningful names. It'd be nice if we could try to dedicate each branch to a single PR and remove the branch afterwards, so as not to produce more and more branches?

patrick-kidger commented 3 years ago

Thanks for the tests and bug fix. The K-S tests fails on my machine. I think it might be worthwhile to think about the pool_size argument for SeedSequence. Using a larger pool_size typically helps in normality, but could also drag down the speed.

Increased (and added an argument) for the pool size. I've set it to 8 by default; as the queries in the BrownianInterval are much shallower than the BrownianTree (which uses pool size 24) then I think this should be fine. (And the KS test now passes; I forgot the seed would make that deterministic.)

I don't actually follow what you were saying about there being a bug in ReverseBrownian?

I fixed this on dev-strat-adjoint and added a docstring; see this. Ultimately, it also has to do with how the adjoint functions are negated. I'm inclined to keep the current form for the adjoints.

Makes sense.

As a minor side note, could we give the branches more meaningful names. It'd be nice if we could try to dedicate each branch to a single PR and remove the branch afterwards, so as not to produce more and more branches?

Aha sorry. I usually tend to end up adding a mix of interdependent things and wanting to rename the branch afterwards, so I've ended up just picking deliberately not meaningful names. Can definitely remove branches afterwards.

patrick-kidger commented 3 years ago

@lxuechen Needing to merge this is holding things up. (In particular the fact that this fixes calling g_prod and therefore being able to use sdeint!) Can we merge it and open the normality discussion as an issue?

lxuechen commented 3 years ago

@lxuechen Needing to merge this is holding things up. (In particular the fact that this fixes calling g_prod and therefore being able to use sdeint!) Can we merge it and open the normality discussion as an issue?

I can merge this after sending another minor commit, but let's keep discussing the problem here, instead of opening up a new issue.