Add tests of various features

[TorkelE]

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 ReactionSystem model. 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.

[ChrisRackauckas]

The tests fail?

[TorkelE]

Not too surprised. I will have a look locally (the test log is too large to actually see what is going on)

[TorkelE]

Ok, tests pass now (except for one downstream test)

[AayushSabharwal]

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 remake should be in SciMLBase's modelingtoolkit_remake.jl testset
• All of the tests for symbolic indexing/interpolation of integrators/solutions/etc should be in a comprehensive_indexing.jl testset
• 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.

[AayushSabharwal]

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

[TorkelE]

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.

[ChrisRackauckas]

this test broken isn't setup right

[AayushSabharwal]

Same for JumpSystems

[TorkelE]

This on fails, but yes, let's investigate closer.

[AayushSabharwal]

Yeah, indexing everywhere should just use getu.

[AayushSabharwal]

Does this still fail? The issue is closed.

[TorkelE]

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.

[TorkelE]

Checked, this still fails for nonlinear integrators

[AayushSabharwal]

Yeah this is a nontrivial change, requiring updates to integrators, solutions, and a generic reimplementation of SII's index provider interface in SciMLBase.

[AayushSabharwal]

This is also a case of everything not using getu.

[AayushSabharwal]

I should have mentioned: I'll move the tests when I PR to SciMLBase to fix the bugs

[TorkelE]

I should have mentioned: I'll move the tests when I PR to SciMLBase to fix the bugs

Thanks!

[TorkelE]

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.

[TorkelE]

For the save_idx I think it is just that it has been broken since the very beginning. It is something that should be supported, so don't think it is bad to keep the tests in until it actually gets fixed, even if it might be a while.

[TorkelE]

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

[TorkelE]

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

[TorkelE]

Haven't heard anything in a while, can we merge this @AayushSabharwal @ChrisRackauckas ?

[ChrisRackauckas]

this isn't actually ran

[ChrisRackauckas]

This needs a bit of work. All of the @test_brokens are not setup correctly and the actual tests are commented.

[TorkelE]

So, there are a large number of cases where I do

    @test_broken false # Does not work for certain inputs, likely related to https://github.com/SciML/ModelingToolkit.jl/issues/2804.
    for u0 in u0_alts_vec, p in p_alts_vec
        oprob = remake(base_oprob; u0, p)
        # @test base_sol == solve(oprob, Tsit5(); saveat = 1.0)
        eprob = remake(base_eprob; u0, p)
        # @test base_esol == solve(eprob, Tsit5(); trajectories = 2, saveat = 1.0)
    end

because some of the tests in the loop are broken, and some are not. Then just doing either @test or @test_broken will cause test failure. Separating the cases will cause tons of line to be written. Hence I comment out the actual test lines, and then write

@test_broken false

to record in the test file that there is something going on, so that the case where something is broken is lost. Similar in cases where the full test is commented out (as in some cases something in the build up fails, and sometimes not).

[ChrisRackauckas]

Then why is it important for this to merge if the test broken isn't actually set to tell us if they are still broken? That's the only point of @test_broken.

[TorkelE]

It is meant as a reminder when you run the tests, it tells you that there are some broken tests and where (and links the issue).

Do you want me to just remove the @test_broken and comment out the relevant bits?

[TorkelE]

@ChrisRackauckas I removed the stuff with the broken test, we can add them back later when they work. This should be good now. @AayushSabharwal do you have any last comments?

[ChrisRackauckas]

Using @test_broken is fine, but the way you did it would not give an appropriate error when the test is fixed, and so it wasn't actually useful. It would be good to add correct @test_brokens where appropriate.