hgrecco / numbakit-ode

Leveraging numba to speed up ODE integration
Other
68 stars 3 forks source link

Fix RungeKutta23 interpolator #17

Open hgrecco opened 3 years ago

hgrecco commented 3 years ago

RungaKutta23 interpolator does not match the SciPy result. Test hast been temporarily disabled.

https://github.com/hgrecco/numbakit-ode/blob/67c186a7053edd832af5cd3d593982e245ea0285/nbkode/testsuite/test_against_scipy.py#L86

Yash-10 commented 3 years ago

Hello @hgrecco I have been looking into this for a comparison with scipy's implementation.

SciPy's implementation uses:

C = np.array([0, 1/2, 3/4])
A = np.array([
    [0, 0, 0],
    [1/2, 0, 0],
    [0, 3/4, 0]
])
B = np.array([2/9, 1/3, 4/9])
E = np.array([5/72, -1/12, -1/9, 1/8])
P = np.array([[1, -4 / 3, 5 / 9],
              [0, 1, -2/3],
              [0, 4/3, -8/9],
              [0, -1, 1]])

whereas `numbakit-ode's implementation uses

C = np.array([0, 1 / 2, 3 / 4], dtype=float)
A = np.array([[0, 0, 0], [1 / 2, 0, 0], [0, 3 / 4, 0]], dtype=float)
B = np.array([2 / 9, 1 / 3, 4 / 9], dtype=float)
B2 = np.array([7 / 24, 1 / 4, 1 / 3, 1 / 8], dtype=float)
P = np.array([[1, -4 / 3, 5 / 9], [0, 1, -2 / 3], [0, 4 / 3, -8 / 9], [0, -1, 1]])

Other implementations such as RK45 and others seem to use the same values but the use of B2 instead of E (with different values) made me wonder whether it is a cause of this issue.

Moreover, I tried to check it locally whether removing B2 and replacing it with E of scipy:

E = np.array([5/72, -1/12, -1/9, 1/8])

solve the error or not. Unfortunately, it still gives an error after running pytest but the difference between nb_y and scipy_sol.y.T seems to have changed.

Could you help me in the above? I am eager to learn more about this implementation.

Thanks!

maurosilber commented 3 years ago

The AdaptiveRungeKutta class (from which RungeKutta23 derives) uses E, which is calculated from B and B2 coefficients here. There seems to be just a difference in sign with respect to Scipy's. But it shouldn't matter, as it is later squared here.

Yash-10 commented 3 years ago

Thank you @maurosilber for your reply and guidance. It seems slightly tricky to figure out the cause of mismatch with the scipy's implementation but I would take a look at it.

Thanks!