Add Wfact and Wfact_t support #149
Conversation
| @test sol.t ≈ sol_Wfact_t.t | ||
| @test sol.u ≈ sol_Wfact_t.u | ||
| else | ||
| @test_broken sol.t ≈ sol_Wfact_t.t |
There was a problem hiding this comment.
that means it's not working, since IIRC at this point only Rosenbrock23 is using the untransformed version still?
There was a problem hiding this comment.
Yeah, as I said, maybe it's a bit too ambitious at this point. The main motivation of the tests is that:
- If we specify the Jacobian, the number of Jacobian and W evaluations should be equal to the non-zero value of our counter, and the solution should be the same.
- If we specify Wfact or Wfact_t, the number of Jacobian evaluations should be zero and the number of W evaluations should be equal to the non-zero value of our counter, and the solution should be the same.
There was a problem hiding this comment.
Oh, I see: the time points are equal, not that the value is correct. Yeah, I am not sure this will be the case, since symbolic CSE stuff happens on the symbolic factorization (the main source of the speedup)
|
What is this waiting for? Should the tests be changed? |
|
I think the tests should be changed: I wouldn't expect W_fact to get the exact same floating point value results, just in the same tolerances. |
|
That said the rest of the PR is fine so I'm willing to merge. |
This PR adds support for
WfactandWfact_t(see #138).I added more tests but probably someone should check if my implementations of
WfactandWfact_tare correct.The tests indicate that the support and statistics for Jacobian and W evaluations can (and should) be improved a lot in OrdinaryDiffEq. Maybe the tests are too ambitious for the current state of the implementation, but at the moment 33 of 68 tests are broken.