Symbolic indexing rework

[AayushSabharwal]

Is this interface appropriate?

[ChrisRackauckas]

Those look good. Is that all that's required for the full interface?

[AayushSabharwal]

I just realised this is a bit of a circular dependency problem. SciMLBase can't use these functions, because MTK depends on it and not the other way around.

[ChrisRackauckas]

Yes, any interface functions must be defined in SciMLBase and extended downstream.

[AayushSabharwal]

Do I move AbstractSystem to SciMLBase too? Or just let the functions be Any-typed

[ChrisRackauckas]

Define stub functions and use them in the interface, and they get extended downstream.

[AayushSabharwal]

With some changes (which I'm cleaning up) symbolic indexing of solutions works. However, DiffEqArrays generated by sol(0.0:1.0:10.0) are not symbolically indexable since they (currently) require all the syms/paramsyms/observed to be passed to them.

[ChrisRackauckas]

Then we should define the interface in RecursiveArrayTools as something about DiffEqArrays and AbstractVectorOfArray, and bubble it up.

[AayushSabharwal]

The solutions I've come up with so far:

• DiffEqArray just takes a scimlfunction instead of syms/observed/etc. This ends up giving it a lot more data than it needs, and is a sort of weak circular dependency so it doesn't seem like the best idea
• DiffEqArray has another field for sys, and uses it if it isn't nothing, basically like how scimlfunctions work.
• RecursiveArrayTools defines a struct SymbolCache (name off the top of my head) containing syms, indepsym and paramsyms which implements the interface in this PR. MTK implements this interface for systems. DiffEqArray now takes anything that implements this interface for symbolic indexing, instead of syms and indepsym. Side effect: DiffEqArrays also allow indexing parameters. I also get the feeling the DiffEqArray indexing code and solution indexing code would be near identical

[AayushSabharwal]

DiffEqArray doesn't seem to support indexing with multiple symbols. The following doesn't work (example uses literal symbols instead of symbolic variables for convenience):

julia> dx = DiffEqArray([[f(x) f2(x)] for x in t], t, [:a,:b])
julia> dx[[:a,:b]]
ERROR: DimensionMismatch: arrays could not be broadcast to a common size; got a dimension with lengths 11 and 2
...

Is this intentional?

[AayushSabharwal]

I've messed about with this a lot (going the SymbolCache route) and it seems independent_variables at the very least needs to be hoisted to RecursiveArrayTools since getindepsym won't work here. At this point, we might as well hoist states and parameters as well.

[ChrisRackauckas]

At that point it's a whole package. SymbolicIndexingInterface.jl? Document it well enough and it can be a thing.

[AayushSabharwal]

That sounds like a good idea. It would be pretty small to start: states, parameters, independent_variables, the new interface described here, and maybe SymbolCache. But I have a feeling we'll end up chucking a lot more in there eventually. Should I just make this work for now (hoisting to RecursiveArrayTools) and refactor into the new package later, or just create the repo now and see where we get.

[ChrisRackauckas]

Make it work, get it tested, and then in a last step refactor to a new package and we move it to SciML to register.

[AayushSabharwal]

SciMLBase tests pass if the following is done:

• Make this branch of MTK use AayushSabharwal/RecursiveArrayTools#symbolcache
• Make SciMLBase use the above branch of RecursiveArrayTools
• Make SciMLBase's test/downstream use this branch of MTK and the above RecursiveArrayTools

[ChrisRackauckas]

Alright cool, seems like a good time to make that extra package then.

[AayushSabharwal]

Everything that needs to be in the package is in src/symbolic_indexing_interface.jl in RecursiveArrayTools. Should be simple enough to move it over. Should I make the package and then transfer it over to SciML?

[AayushSabharwal]

Voila: https://github.com/AayushSabharwal/SymbolicIndexingInterface.jl

[AayushSabharwal]

Initiated the transfer. Github says it might take a couple minutes

[AayushSabharwal]

Should I tag a release for SymbolicIndexingInterface, so the other packages can depend on it?

[ChrisRackauckas]

I set it to go.

[AayushSabharwal]

This should work. For some reason I can't install SymbolicIndexingInterface locally yet. It won't work in CI, however, since it doesn't know to depend on the specific SciMLBase and RecursiveArrayTools branches without Manifest.toml

[AayushSabharwal]

Can I rename the other states functions (which aren't intended to implement the interface) to _states (such as the one in src/systems/connectors.jl:164)? Or do I just overload it and ignore the fact that it's not the intended use of the function? Fairly certain we need one or the other, since the interface states method is used a lot and adding SymbolicIndexingInterface. to all those places seems a bit redundant. It's also why precompilation will fail.

[ChrisRackauckas]

Can I rename the other states functions (which aren't intended to implement the interface) to _states (such as the one in src/systems/connectors.jl:164)?

yes

[AayushSabharwal]

Is there a case where states(sys) has two variables, but size(sol, 1) == 1? It's happening for test/components.jl:100. states(sys2) == [capacitor.v, capacitor.i] but size(sol2) == (1, 15)

EDIT: Same thing in test/structural_transformation/tearing.jl:179

[AayushSabharwal]

Everything should work now, bar the tests mentioned above, if the relevant branches of the other repositories are used. I haven't been able to run the distributed tests since Distributed tends to segfault on my system for reasons I haven't figured out yet.

[AayushSabharwal]

Should this PR still pass syms/indepsym/paramsyms to SciMLFunctions? It doesn't currently, but I realised it probably should so it can be merged without SciMLBase, then after SciMLBase gets merged they can be removed in a separate PR to make 100% sure everything works.

[ChrisRackauckas]

Yes, and then do an upper bound in the second PR. That way it's all done safely.

[AayushSabharwal]

Done. Now SciMLBase can be updated to 1.80 without MTK trying to use it

EDIT: Wait, I got it wrong. There's no need to restrict this since SciMLBase should support the old method anyway

[AayushSabharwal]

Once this merges, SciMLBase can depend on it, so those changes can be tested and then merged