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

Dev kidger2 #19

Closed patrick-kidger closed 3 years ago

patrick-kidger commented 3 years ago

Quite a lot of bugfixes, mostly.

List - Updated diagnostics. - Removed trapezoidal_approx - Fixed error messages for wrong methods etc. - Can now call BrownianInterval with a single value. - Fixed bug in BrownianInterval that it was always returning 0! - There's now a 2-part way of getting levy area: it has to be set as available during `__init__`, and then specified that it's wanted during `__call__`. This allows one to create general Brownian motions that can be used in multiple solvers, and have each solver call just the bits it wants. - Bugfix spacetime -> space-time - Improved checking in check_contract - Various minor tidy-ups - Can use Brownian* with any Levy area sufficient for the solver, rather than just the minimum the solver needs. - Fixed using bm=None in sdeint and sdeint_adjoint, so that it creates an appropriate BrownianInterval. This also makes method='srk' easy.

The diagnostics and sdeint(bm=None) both use BrownianInterval just because that's the only one offering U at the moment.

Also, whilst its not quite a formal test, the diagnostics seem positive about BrownianInterval; in particular it looks like SRK vs analytic is consistent, which implies that the space-time Levy area is likely correct wrt increment.

Btw, what name would you use for U? I'd describe it as "a cross term in the second order signature", but that's not very snappy and I'd like to replace LEVY_AREA_APPROXIMATIONS.spacetime with a more appropriate name.

lxuechen commented 3 years ago

Btw, what name would you use for U? I'd describe it as "a cross term in the second order signature", but that's not very snappy and I'd like to replace LEVY_AREA_APPROXIMATIONS.spacetime with a more appropriate name.

I'm somewhat unsure about this. I think we could leave it as space-time for now unless you have a better idea. I was gonna call it "integrated_brownian", but this term is too long and not very accurate.

lxuechen commented 3 years ago

As a side comment, did you make sure all of the code in diagnostics, examples still run?

I can fix the tests later on.

lxuechen commented 3 years ago

I realized this PR might conflict with the PR in #20 slightly. I can merge this after the comments are resolved, and feel free to leave plenty of comments on my PR.

For now, I'm moving forward with the adjoint for general noise Strat SDE.

patrick-kidger commented 3 years ago

Btw, what name would you use for U? I'd describe it as "a cross term in the second order signature", but that's not very snappy and I'd like to replace LEVY_AREA_APPROXIMATIONS.spacetime with a more appropriate name.

I'm somewhat unsure about this. I think we could leave it as space-time for now unless you have a better idea. I was gonna call it "integrated_brownian", but this term is too long and not very accurate.

Area under curve - auc - maybe? Not sure I like that either really.

As a side comment, did you make sure all of the code in diagnostics, examples still run?

I can fix the tests later on.

I checked that benchmark/brownian.py, diagnostics/ito_additive.py and diagnostics/ito_diagonal.py run. I found that diagnostics/ito_scalar.py takes a long time and eats up memory, even when using BrownianPath rather than BrownianInterval. I've not dug into why that is. I think some of the examples might need updating, but I'll check those and report back.

patrick-kidger commented 3 years ago

latent_sde.py seems to run correctly. Other than evaluting BrownianInterval outside of t0, t1, I think that's everything.

lxuechen commented 3 years ago

Ok, the merge looks good.

Issues we should still keep track of are