Regression: Problem with indexing of JumpProblem

[TorkelE]

Also one case which did not work still does.

using ModelingToolkit, JumpProcesses

@parameters p d
@variables t X(t)
rate1   = p
rate2   = X*d
affect1 = [X ~ X + 1]
affect2 = [X ~ X - 1]
j1 = ConstantRateJump(rate1, affect1)
j2 = ConstantRateJump(rate2, affect2)

# Works.
@mtkbuild js = JumpSystem([j1, j2], t, [X], [p,d])
dprob = DiscreteProblem(js, [X => 15], (0.0, 10.), [p => 2.0, d => 0.5])
jprob = JumpProblem(js, dprob, Direct())
sol = solve(jprob, SSAStepper())

jprob[X]
jprob[[X]] # Error (regression)
jprob[(X,)] # Error, did error previously.

[AayushSabharwal]

Yeah, JumpProcesses should not have a getindex method for JumpProblem. Those tests are marked as broken

[TorkelE]

Glad to hear that it is under control. I am a bit confused though, was this feature intentionally withdrawn for now? Maybe more importantly, it is intended to re introduce this type of indexing?

[AayushSabharwal]

It wasn't intentionally withdrawn, just that it was in a way inevitable. It's a problem sort of unique to Julia. There are two ways out of this:

1. JumpProcesses removes the getindex method
2. The getindex method in SciMLBase is redefined to apply to apply to all of these types except AbstractJumpProblem:

julia> subtypes(SciMLBase.AbstractSciMLProblem)
5-element Vector{Any}:
 SciMLBase.AbstractDEProblem
 SciMLBase.AbstractEnsembleProblem
 SciMLBase.AbstractIntegralProblem
 SciMLBase.AbstractLinearProblem
 SciMLBase.AbstractOptimizationProblem

julia> subtypes(SciMLBase.AbstractDEProblem)
9-element Vector{Any}:
 SciMLBase.AbstractDAEProblem
 SciMLBase.AbstractDDEProblem
 SciMLBase.AbstractJumpProblem
 SciMLBase.AbstractNoiseProblem
 SciMLBase.AbstractNonlinearProblem
 SciMLBase.AbstractODEProblem
 SciMLBase.AbstractPDEProblem
 SciMLBase.AbstractRODEProblem
 SciMLBase.AbstractSDDEProblem

[AayushSabharwal]

This sort of thing crops up a lot in Julia, where you want a subtype to just wrap another type and yet forwarding the method can lead to unintentional ambiguities down the line, despite the fact that no semantic guarantees were broken

[TorkelE]

got it, makes sense. thanks again for helping improve the indexing test :)

[ChrisRackauckas]

The getindex method in SciMLBase is redefined to apply to apply to all of these types except AbstractJumpProblem:

We might want to do something of the sort. In particular, some AbstractConcreteSciMLProblem or something, and then better define the interface on that, where everything has a u0, etc. This would be a good part of making our interface definitions more exact.

[AayushSabharwal]

Changing the type hierarchy is breaking, though?

[ChrisRackauckas]

No, we'd keep AbstractSciMLProblem the same, we'd just add something below it that makes more assumptions and update the dispatches to those that require those assumptions. AbstractConcreteSciMLProblem <: AbstractSciMLProblem is assumed.

[AayushSabharwal]

If AbstractDEProblem <: AbstractConcreteSciMLProblem <: AbstractSciMLProblem then AbstractJumpProblem would have to stop subtyping AbstractDEProblem. If AbstractConcreteSciMLProblem <: AbstractDEProblem <: AbstractSciMLProblem then it doesn't really make sense because there are concrete problems which are not DE problems. The concept of concreteness and the categorization we have here are orthogonal

[ChrisRackauckas]

If AbstractDEProblem <: AbstractConcreteSciMLProblem <: AbstractSciMLProblem then AbstractJumpProblem would have to stop subtyping AbstractDEProblem.

Hmm... we'd need to hash out the whole plan, but I think it would be good to spend the time to do this and then actually define what these abstract types stand for.

[ChrisRackauckas]

@AayushSabharwal did this get addressed?

[AayushSabharwal]

No. I think the easiest solution is for JumpProcesses to not implement getindex. I'll PR to make that happen

[AayushSabharwal]

SciML/JumpProcesses.jl#469 is merged

[isaacsas]

Sorry all, I somehow missed this discussion (otherwise I would have updated JumpProcesses for this long ago). Feel free to ping me on such issues in the future!

[AayushSabharwal]

In all fairness, I should've made the PR when this issue was opened.