search

google-research / torchsde

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

Made `BrownianTree` deterministic wrt entropy #7

Closed patrick-kidger closed 4 years ago

patrick-kidger commented 4 years ago

Thanks for the library, I'm enjoying using it.

Fixed the issue in the title. I've not tried modifying the corresponding C++: the code for determinism wrt seeds looks a bit more finickity there.

lxuechen commented 4 years ago

Thanks for catching this problem and quickly coming up with a patch!

I think there's perhaps a deeper problem here. Since the queries outside of the range of [t0, t1] are based on the BrownianPath logic, there currently are no guarantees that the global entropy will determine what the sampled values are out there.

I think the present fix is good enough to be merged. But I'll open up a separate issue discussing the general problem. I think the case is rare enough that people probably won't actively ask for values outside [t0, t1], if they were explicitly giving t0 and t1 to the data structure. Though, at least there should be some warning or documentation on this.

patrick-kidger commented 4 years ago

Actually, I think it's worse than that! When I've queried with t > t1 (by accident), I actually observe that None is returned, e.g.

torchsde.BrownianTree(t0=torch.tensor(0.), w0=torch.tensor(0.))(2.)

Not helped by the implicit assumption that t1 = t0 + 1.

lxuechen commented 4 years ago

I think you're right, this is a problem which I've just reproduced. Could you open up another issue? I can fix this right away. I also had plans for enhancing the tests for the brownian components. I think this should be done in a unified commit.

Thanks again for pointing this out!

lxuechen commented 4 years ago

As a side note, I'll also send in a patch for the C++ side. That part was written somewhat hastily, so definitely no worries for not walking through that!