Skip to content

Conversation

sammccallum
Copy link

@sammccallum sammccallum commented Mar 9, 2025

Fixes sign of h in AbstractSRK. See #598.

@patrick-kidger
Copy link
Owner

LGTM!
Can you (a) target dev and (b) add a test? We can bundle this in to the upcoming #600 and make this part of the next release :)

@sammccallum
Copy link
Author

sammccallum commented Mar 10, 2025

Right, turns out this is a slightly more involved bug! The Lévy area term $H_{s, t}$ isn't correctly handled when solving backwards-in-time too. I believe this is due to VBT returning $-H_{s, t}$ (along with $-W_{s, t}$ and $-K_{s, t}$) whereas $H_{s, t}$ should remain unchanged in sign.

I think there is a choice for whether this should be directly handled by VBT or by the solvers themselves? Tagging 'the SDE lads' @andyElking and @lockwo for their thoughts?

Btw, the original change (dt -> -dt) fixes all the solvers to converge backwards-in-time but with strong order 1.0 for additive noise (because we need to handle $H_{s, t}$ as well)!

@andyElking
Copy link
Contributor

Thanks for spotting this, Sam. Idk how the h = t0 - t1 slipped past me, it's just clearly wrong.

In terms of VBT, I think _tree._levy_diff should handle this correctly. I think the issue is that WrapTerm just blanket multiplies contr with self.direction, but H should in that case be multiplied by self.direction ** 2 (i.e. be left untouched) (and K by self.direction ** 3, but that amounts to the same). I could be wrong. @sammccallum can you please check if this is what's causing the issue? I am very busy this week, but if this doesn't fix it, I can probably take a closer look next week.

@sammccallum
Copy link
Author

Yeah, that's what I thought and it semi-works: flipping the sign of H after it is returned from diffusion.contr gets all the additive noise methods (SEA, SRA1, ShARK) back to order 1.5. But the commutative noise methods (GeneralShARK, SlowRK, SPaRK) are still stuck at order 1.0 (for an additive noise SDE).

@patrick-kidger
Copy link
Owner

Okay! I'll let you folks figure out how you want to tackle this, and since it looks like that might take a little time then I'll do a release on #600 in the mean time anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants