Add tests of various features #2806
Conversation
|
The tests fail? |
|
Not too surprised. I will have a look locally (the test log is too large to actually see what is going on) |
|
Ok, tests pass now (except for one downstream test) |
There was a problem hiding this comment.
Thanks for the extremely comprehensive tests. Apart from the specific comments below, I have a couple of overarching requests.
Firstly, these tests don't belong in MTK. They are all testing features implemented in SciMLBase, and should go there alongside the other tests for similar functionality.
- Anything to do with
remakeshould be in SciMLBase'smodelingtoolkit_remake.jltestset - All of the tests for symbolic indexing/interpolation of integrators/solutions/etc should be in a
comprehensive_indexing.jltestset - Both of the above should be in the
"SymbolicIndexingInterface"group
That way all of these will be run in MTK downstream, and are tested in the library that implements the features being tested. SciMLBase wants to run these tests because changes in SciMLBase are more likely to break them than ones in MTK. remake for example is almost entirely a SciMLBase feature, and the only places MTK is likely to break it is remake_buffer.
Testing save_idxs is also unnecessary in my opinion. It's not something that's broken, it's a feature we don't support yet.
I would also like to not merge these tests in a state where the broken ones are commented out/marked as @test_broken. That defeats the purpose of the tests. We should test everything, and merge when it's all fixed.
| base_esol = solve(base_eprob, ImplicitEM(); seed, trajectories = 2, saveat = 1.0) | ||
|
|
||
| # Simulates problems for all input types, checking that identical solutions are found. | ||
| @test_broken false # first remake in subsequent test yields a `ERROR: type Nothing has no field portion`. |
There was a problem hiding this comment.
What's the difference between this SDESystem and the one in https://github.com/SciML/SciMLBase.jl/blob/master/test/downstream/modelingtoolkit_remake.jl? remake passes there and fails here
There was a problem hiding this comment.
I think this ones goes through a couple of additional cases (e.g. observables). not sure why this one errors though, probably something very niche.
| base_esol = solve(base_eprob, SSAStepper(); seed, trajectories = 2, saveat = 1.0) | ||
|
|
||
| # Simulates problems for all input types, checking that identical solutions are found. | ||
| @test_broken false # first remake in subsequent test yields a `ERROR: type Nothing has no field portion`. |
There was a problem hiding this comment.
This on fails, but yes, let's investigate closer.
test/sciml_struct_interfacing.jl
Outdated
| @test prob[X] == prob[sys.X] == prob[:X] == 4 | ||
| @test prob[XY] == prob[sys.XY] == prob[:XY] == 9 | ||
| @test prob[[XY, Y]] == prob[[sys.XY, sys.Y]] == prob[[:XY, :Y]] == [9, 5] | ||
| @test_broken prob[(XY, Y)] == prob[(sys.XY, sys.Y)] == prob[(:XY, :Y)] == (9, 5) # https://github.com/SciML/SciMLBase.jl/issues/709 |
There was a problem hiding this comment.
Yeah, indexing everywhere should just use getu.
test/sciml_struct_interfacing.jl
Outdated
|
|
||
| # Test integrator indexing. | ||
| let | ||
| @test_broken false # NOTE: Multiple problems for `nint` (https://github.com/SciML/SciMLBase.jl/issues/662). |
There was a problem hiding this comment.
Does this still fail? The issue is closed.
There was a problem hiding this comment.
The issues got closed but then the conversation continued. But I see that there was a PR merged related. I checked fairly recently and this was still broken, but should check again.
There was a problem hiding this comment.
Checked, this still fails for nonlinear integrators
test/sciml_struct_interfacing.jl
Outdated
| end | ||
|
|
||
| # Test solve's save_idxs argument. | ||
| # Currently, `save_idxs` is broken with symbolic stuff (https://github.com/SciML/ModelingToolkit.jl/issues/1761). |
There was a problem hiding this comment.
Yeah this is a nontrivial change, requiring updates to integrators, solutions, and a generic reimplementation of SII's index provider interface in SciMLBase.
test/sciml_struct_interfacing.jl
Outdated
|
|
||
| # Handles nonlinear and steady state solutions differently. | ||
| let | ||
| @test_broken false # Currently a problem for nonlinear solutions and steady state solutions (https://github.com/SciML/SciMLBase.jl/issues/720). |
There was a problem hiding this comment.
This is also a case of everything not using getu.
|
I should have mentioned: I'll move the tests when I PR to SciMLBase to fix the bugs |
Thanks! |
|
Yeah, I am a bit uncertain what actually goes where now when the MTK pipeline have been split up in so many different places. To me it is basically tests of the System types that MTK implements. But I agree that if possible it would make sense to move them as far internal as possible. If you point out what and where I could do it, but might be easier for you to do it yourself. |
|
For the |
|
Are we sure that the modified version of these that got ported to SciMLBase are actually exhaustive? I.e. regression that I caught with these tests in Catalyst still appear in the ecosystem: #2838 |
|
I have modified this one, removing the SciMLBase related stuff, and also adding tests that static vectors can be used as input across all problem types. @AayushSabharwal |
Ok, I have tried to rewrite various tests in Catalyst to work within MTK (a bit more complicated since each system have to be generated directly, and cannot be generated from a single
ReactionSystemmodel. These tests are primarily going through vaiours ways to interact with SciML structures, and giving input to problems, across various system types.I have tried to go through all broken tests, and mark issues that are related. Unfortunately there are quite a few, so someone else would probably need to have a second look at each to check the issues are accurate.