-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
0.2.0 TODO list #15
Comments
@lxuechen Benchmarks look rather encouraging for the BrownianInterval. e.g. see below. (Every benchmark exhibits the same behaviour.) I would note that the number of time steps is very low in the benchmarks (only 100). I expect that larger timesteps will result in the BrownianPath overtaking the BrownianInterval. |
This is encouraging! Would you have an idea as to where the gain is coming from? I would guess it's coming from 1) using How much different are the plots when these bms are plugged in the solvers? |
I can think of a couple other things for certain cases:
The equivalent plots for the solvers look pretty much identical; I just picked one at random. |
Several side comments:
This breaks backwards compatibility. I'd be nice if we could have
This line might be wrong if the second output of A side comment is that I don't totally support merging the SRK methods for now. But I think we should be able to get rid of the tableuas folder if you find it disturbing. Also the docstring needs to change for that file. |
Done.
I'll add that to the bug list!
The SRK methods actually aren't merged at the moment; for now at least I've taken the reasonably straightforward approach of unifying as much of the easy code as I can and leaving their
Done! |
I see. For GPUs the copying part makes sense. The list inserts argument doesn't seem to be the most convincing to me, since blist should have O(logn) complexity. |
Overall seems like awesome progress! |
Good point about the blist. Thanks. I'm expecting to try and tackle most of the bugs first. I've just seen your email so I'll continue responding over there. |
I'd recommend we first test BrownianInterval and check for its correctness. This actually should have been done before rewriting the solvers. The problem is that now, I couldn't run the rate inspect diagnostics, since the BInterval is raising errors.
I can do this. |
One of the reasons I had it in a separate branch originally!
Excellent, thankyou! |
If my derivation is correct then I think the term needed in the SRK should be:
But it's also 1am where I am. Does that look about right to you? |
Yes, this is right. |
I think, for the moment, I wouldn't worry too much about rewriting/refactoring more of the existing code. The thing that I'm worried more about is whether the new This also relates to the habit of mine where I'd like to run the test suit every time I make a minor change, so that I know exactly what could go wrong. |
Here are tests that I think would benefit us when we go forward for
I think once these are in place, I'd have much more confidence in going forward. |
Another note is that is there a way that we can simplify the interface, as right now the user is basically forced to manually supply levy_area_approx='davie'/'foster'/'space-time' just in order to use SRK. Using a high order scheme shouldn't be this difficult! Also, does Davie approximation have any advantages over Foster? I thought Foster's version is more accurate, at the cost of one extra random number generation, which is totally reasonable considering the costs of other components. |
Your concerns about tests are reasonable - bugs and tests are definitely the next things to handle. I agree that having to specify levy area to use SRK should be fixed; this is already on the bug list. I'm not sure what the best way to handle this is. The alternative option is slightly messier, but still doable - Care to make a judgement one way or the other? Davie's approximation is slightly faster than Foster, by a bit more than just the random number generation (there's a bit of algebra too), but AFAIK there's not a big difference between them however you slice it. But for forward compatibility with even better approximations in the future, I'd avoid code layouts for which Foster gets baked in as the only option. |
I agree with this, and the plan seems nice.
SGTM. |
@lxuechen This is labelled as Ito-specific: torchsde/torchsde/_core/base_sde.py Line 53 in 133a745
Is there any part of that which actually relies on that though? It looks like it's just general drift/diffusion operations. |
To-do list so far:
Brownian motions:
calls inAs of Dev kidger2 #19 support single-point evaluationexamples/demo.ipynb
calls inAs of Dev kidger2 #19 support single-point evaluationtests/problems.py
tests/test_brownian_path.py
tests/test_brownian_tree.py
trapezoidal_approx
with integrated Brownian bridge for speed #12)trapezoidal_approx
with integrated Brownian bridge for speed #12) (Done in Dev kidger2 #19)__init__
:t
outside of t0, t1. (Patrick) (Global entropy doesn't determine sampled values outside of [t0, t1] forBrownianTree
#9, CallingBrownianTree
witht > t1
returnsNone
#10, BrownianInterval tests + bugfixes #28)Solvers:
integrate_logqp
andinterp_logqp
SDEs:
gdg_prod
for the adjoint. (Chen)sde_type
and themethod
.Misc:
python_requires
.diagnostics/ito_scalar.py
appears to be using huge amount of memory, and is also using a diagonal (and thus not compatible) problem.sdeint
,sdeint_adjoint
to automatically select the probably-best solver for a given noise type / SDE type combination. (Rather than just always using SRK.) (Dev methods fixes #73)Tests:
test_sdeint.py
,test_adjoint.py
,test_adjoint_logqp.py
(Chen) (Add support for Stratonovich adjoint #21)test_brownian_interval.py::test_normality_conditional
(BInterval - U fix #44)test_sdeint.py
to pytest. (Dev methods fixes #73)Bugs to fix:
a
from space-time Levy area to Levy area is of the wrong shape. (Patrick) (BrownianInterval tests + bugfixes #28)I_k0
in SRK.method='srk'
andlevy_area_approximation='none'
are incompatible with one another. (Done in Dev kidger2 #19)bm
isn't passed then it defaults to being aBrownianPath
on the CPU, potentially causing device errors later. (Done in Dev kidger2 #19)The text was updated successfully, but these errors were encountered: