Skip to content
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

Srk table #469

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Srk table #469

wants to merge 3 commits into from

Conversation

andyElking
Copy link
Contributor

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 :)

@patrick-kidger
Copy link
Owner

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.

@andyElking
Copy link
Contributor Author

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.

@lockwo
Copy link
Contributor

lockwo commented Aug 1, 2024

Should ralston and EulerHeun also be in the table?

@andyElking
Copy link
Contributor Author

Should ralston and EulerHeun also be in the table?

Good point, I'll add them.

@andyElking
Copy link
Contributor Author

I added Ralston and EulerHeun to the table.

@patrick-kidger
Copy link
Owner

patrick-kidger commented Aug 5, 2024

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? :)

@andyElking andyElking force-pushed the srk_table branch 2 times, most recently from e959929 to 063d4e7 Compare August 7, 2024 15:36
@andyElking
Copy link
Contributor Author

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.

@lockwo lockwo mentioned this pull request Aug 15, 2024
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