diffrax icon indicating copy to clipboard operation
diffrax copied to clipboard

Srk table

Open andyElking opened this issue 1 year ago • 11 comments

Hi Patrick, this is a lightweight PR, adding some more SRK documentation. The first commit just adds the table of all SDE solvers into docs/api/solvers/sde_solver.md. The second commit adds a Jupyter notebook to examples/, wherein I gave a walkthrough of the basics SDE simulation in Diffrax. Feel free to get rid of the second commit if you do not find it necessary :)

andyElking avatar Jul 29 '24 16:07 andyElking

So I'm feeling this table is maybe better-suited to the devdocs. It's a useful reference but this is large enough that I don't think I can commit to ensuring its accuracy + that it remains up-to-date. (We can add a reference to it from the main SDE docs, whilst mentioning the above caveats.)

As for the example -- note that WeaklyDiagonalControlTerm is now deprecated in favour of a ControlTerm returning a lx.DiagonalLinearOperator, so we should avoid recommending it. My main comment here is that I think you need to kill some darlings -- I try to keep the examples fairly short to ensure readability.

patrick-kidger avatar Jul 31 '24 14:07 patrick-kidger

I removed some things from sde_example.ipynb and moved it to devdocs. I removed the table from sde_solvers.md, but I added a link pointing to the table at the bottom of sde_example.ipynb.

I feel like it is important to at least somewhere in the docs show users the canonical way of batch-solving SDEs, because Monte Carlo is the primary use case of SDE simulation. This is why I think we should really keep that part of the notebook.

andyElking avatar Jul 31 '24 18:07 andyElking

Should ralston and EulerHeun also be in the table?

lockwo avatar Aug 01 '24 00:08 lockwo

Should ralston and EulerHeun also be in the table?

Good point, I'll add them.

andyElking avatar Aug 01 '24 16:08 andyElking

I added Ralston and EulerHeun to the table.

andyElking avatar Aug 04 '24 16:08 andyElking

Okay, I like this! I should have clearer in my past message by the way, I think only the table of SDE information needs to be in the devdocs (and it should be included in mkdocs.yml). I think the notebook you have here is really useful, so I'd actually like to include that in the main example docs!

I don't think GitHub lets me comment on notebook files so I'll leave review feedback here instead:

  • can you tidy up the Diffrax imports, so that it doesn't appear twice?
  • Can you standardise the notation to the same used throughout the rest of the Diffrax documentation, e.g. see here: https://docs.kidger.site/diffrax/usage/how-to-choose-a-solver/#stochastic-differential-equations (Note how I use y instead of Y, w instead of W etc -- the style and characters I use are chosen to allow using equivalent notation for ODEs/SDEs/CDEs interchangably.)
  • I'd like to title this 'Advanced SDE example' since it includes topics like Levy area, Monte Carlo etc.
  • Let's have just print(f"Minimal levy area for SPaRK: {solver.minimal_levy_area}.") (no __name__) -- I prefer simplicity of code over simplicity of printout in these examples.
  • It is very important to have h > bm_tol -- can you briefly add a sentence explaining why?
  • I'd really prefer for examples to not rely on private functionality, or functions defined in tests. Let's rewrite the 'optimizing wrt SDE parameters' section to be a bit simpler, focusing on just the optimization?
  • For the ito-vs-stratonovich, different noise types, etc., then I'd like to avoid duplicating the information already in https://docs.kidger.site/diffrax/usage/how-to-choose-a-solver/#stochastic-differential-equations -- can you just link to that instead? (And add any extra information there if you think something is missing.)

WDYT? :)

patrick-kidger avatar Aug 05 '24 15:08 patrick-kidger

Hi Patrick!

I agree with all of your comments and I made the appropriate modifications. Two notes however:

  • In the definition of commutative noise you originally used \mu and \sigma. Because I instead use f and g everywhere else, including in AbstractSRK documentation, I opted to just convert everything to f and g. I assume you don't mind the change too much as long as the notation is consistent everywhere. LMK
  • I agree I shouldn't use a private method such as _batch_sde_solve in the example, so I just wrote an equivalent in the notebook. However, several people (including two of James's new students and another person at the Edinburgh conference) were asking me about canonical ways to do Monte Carlo with Diffrax, so we could consider writing an equivalent of _batch_sde_solve in Diffrax proper and making it public. I don't feel very strongly about this though.

Please let me know if I missed anything or if you notice anything else I should change.

andyElking avatar Aug 07 '24 15:08 andyElking

Hi @patrick-kidger,

Is this still relevant? If yes, I am happy to rebase it to the most recent version, etc. Otherwise we can just close it.

andyElking avatar Sep 18 '25 19:09 andyElking

I think this is still relevant! One of the smaller PRs that I've never found time to follow-up on 😅 I think go ahead and rebase this and I'd be happy to get this in :)

patrick-kidger avatar Oct 03 '25 12:10 patrick-kidger

Just rebased it, the notebook still runs perfectly without requiring any changes.

andyElking avatar Oct 09 '25 01:10 andyElking

Just fyi the errors that pyright is reporting have nothing to do with the files I changed (this is a purely docs-related PR, so shouldn't really require tests...)

andyElking avatar Oct 09 '25 01:10 andyElking