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

Add Riccati ODE solver to dsolve benchmarks #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

naveensaigit
Copy link

@naveensaigit
Copy link
Author

@oscarbenjamin Some existing tests are failing. I fixed the failure in test_solve.py, but I'm unable to understand the dsolve systems test which is failing. Could you please take a look?

@oscarbenjamin
Copy link
Contributor

Are you asking about this?

    def test_make_ode_01():
        ode, params = _make_ode_01()
        t, y, y0, k = params
        result = dsolve(ode, y[1](t))
        eq_assumption = sympy.Q.is_true(Ne(k[1], k[0]))
        refined = refine(result, eq_assumption)
        ignore = k + y0 + (t,)
>       int_const = [fs for fs in refined.free_symbols if fs not in ignore][0]
E       AttributeError: 'list' object has no attribute 'free_symbols'

benchmarks/tests/test_dsolve.py:15: AttributeError

I think that what has happened is that dsolve now returns a list where it previously returned a single Eq.

@naveensaigit
Copy link
Author

Are you asking about this?

Yes. Now dsolve returns two solutions and both of them are Piecewise. The expected solution doesn't seem to be equal to either of them.

@oscarbenjamin
Copy link
Contributor

The dsolve change is bisected to sympy/sympy@6318773 from sympy/sympy#21124

The previous output was:

In [1]: v = Function('v')

In [2]: t, k0, k1, u0 = symbols('t, k0, k1, u0')

In [3]: eq = Eq(Derivative(v(t), t), k0*u0*exp(-k0*t) - k1*v(t))

In [4]: eq
Out[4]: 
d                 -k₀⋅t          
──(v(t)) = k₀⋅u₀⋅      - k₁⋅v(t)
dt                               

In [5]: dsolve(eq)
Out[5]: 
       ⎛           ⎛⎧        k₁⋅t                    ⎞⎞       
       ⎜           ⎜⎪      -                        ⎟⎟       
       ⎜           ⎜⎪───────────────────  for k₀ ≠ k₁⎟⎟  -k₁⋅t
v(t) =C+ k₀⋅u₀⋅⎜⎨    k₀⋅t       k₀⋅t             ⎟⎟⋅     
       ⎜           ⎜⎪k₀⋅     - k₁⋅                 ⎟⎟       
       ⎜           ⎜⎪                                ⎟⎟       
       ⎝           ⎝⎩         t            otherwise ⎠⎠ 

The new output is

In [5]: dsolve(eq)
Out[5]: 
⎡                                                        ⎧       C₁⋅kC₁⋅kk₀⋅u₀                    ⎤
⎢       ⎧    -k₀⋅t            -k₀⋅t                      ⎪─────────────────── - ─────────────────── - ───────────────────  for k₀ ≠ k₁⎥
⎢       ⎪C₁⋅      + k₀⋅tu₀⋅       for k= k₁         ⎪    k₁⋅t       k₁⋅t       k₁⋅t       k₁⋅t       k₀⋅t       k₀⋅t             ⎥
⎢v(t) = ⎨                                       , v(t) =k₀⋅     - k₁⋅       k₀⋅     - k₁⋅       k₀⋅     - k₁⋅                 ⎥
⎢       ⎪           nan               otherwise          ⎪                                                                            ⎥
⎢       ⎩                                                ⎪                              nan                                 otherwise ⎥
⎣                                                        ⎩                                                                            ⎦

This is really just a convoluted way of writing the same thing. If k0 == k1 then the first solution is equal to the corresponding case from the Piecewise and the second solution is nan. If k0 != k1 then it's the other way round. They might look different but they are equivalent e.g.:

In [15]: sol1, sol2 = dsolve(eq)

In [16]: sol2.rhs.args[0][0]
Out[16]: 
       C₁⋅kC₁⋅kk₀⋅u₀       
─────────────────── - ─────────────────── - ───────────────────
    k₁⋅t       k₁⋅t       k₁⋅t       k₁⋅t       k₀⋅t       k₀⋅t
k₀⋅     - k₁⋅       k₀⋅     - k₁⋅       k₀⋅     - k₁⋅    

In [17]: sol2.rhs.args[0][0].collect(Symbol('C1'), cancel)
Out[17]: 
    -k₁⋅t          k₀⋅uC₁⋅      - ───────────────────
                k₀⋅t       k₀⋅t
            k₀⋅     - k₁⋅  

Of course the single Piecewise solution is the preferred form so this is a regression. Looks related to the comment here: sympy/sympy#21124 (comment)

@oscarbenjamin
Copy link
Contributor

I'm really not sure why that dsolve example is tested here in the benchmark test suite though.

@asmeurer
Copy link
Member

I guess the benchmarks are tested to make sure they don't break, and also so you can run the tests against an older version of SymPy to ensure that the benchmarks actually do benchmark the right thing. The latter case is more important in my opinion. If we want to guard against regressions, we should add the tests to the SymPy test suite. And if the goal is just to be able to check validity of the benchmark, the tests here should be written in a much more generic way. Instead of asserting an exact solution, it should only check that it is mathematically correct, e.g., using checkodesol, or using some sort of numerical consistency checks for non-solver benchmarks where this isn't possible.

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