DiffEqML / torchdyn

A PyTorch library entirely dedicated to neural differential equations, implicit models and related numerical methods
https://torchdyn.org
Apache License 2.0
1.35k stars 125 forks source link

save at for fixed odeint #136

Closed joglekara closed 2 years ago

Zymrael commented 2 years ago

Looks good, can we add a small test for this?

joglekara commented 2 years ago

@Zymrael 👍🏾 will add a small test!

@massastrello Good catch, and excellent point, I love the indexing-based solution that avoids the condition evaluation

joglekara commented 2 years ago

@massastrello on second thought, the indexing approach doesn't help here because the goal is to no longer store the full state along the integration path. For motivation, the state of some PDE solves relevant to plasma physics can be O(GB) in which case we can only integrate, and more crucially, perform AD against, a few timesteps at a time.

The way I see it, there are two sides to the spectrum

1) where solver.step() is expensive and x is large. In those situations, it is not expensive to run the if call within the loop, while you are prohibited from a longer time-scale integration because of the need to store the state. 2) where solver.step() is cheap and x is small in which case we can process the stored quantities after the loop

I'm inclined to think the if + sum() call may not be so expensive anyway because t_span is at worst still a 1D quantity but curious to hear what you think

Zymrael commented 2 years ago

@massastrello point is that regardless of how we check t_span vs t_store, the latter should always be (in fixed-step solvers) a strict subset of the former; basically a config for checkpoints. However, I don't think allclose makes any significant difference since as you mentioned t_span is a 1D array (of course the best approach here would be to have better benchmarking to settle these discussions with hard measurements).

For adaptive-steppers, interpolation allows us to save at "arbitrary" times. In those cases, allclose checks are definitely required.

joglekara commented 2 years ago

done

massastrello commented 2 years ago

Thanks! Looks good to me.