ModelingToolkit.jl icon indicating copy to clipboard operation
ModelingToolkit.jl copied to clipboard

Regression: Problem with indexing of JumpProblem

Open TorkelE opened this issue 1 year ago • 10 comments

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.

TorkelE avatar Jul 02 '24 00:07 TorkelE

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

AayushSabharwal avatar Jul 02 '24 06:07 AayushSabharwal

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?

TorkelE avatar Jul 02 '24 16:07 TorkelE

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 avatar Jul 02 '24 17:07 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

AayushSabharwal avatar Jul 02 '24 17:07 AayushSabharwal

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

TorkelE avatar Jul 02 '24 23:07 TorkelE

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.

ChrisRackauckas avatar Jul 03 '24 07:07 ChrisRackauckas

Changing the type hierarchy is breaking, though?

AayushSabharwal avatar Jul 03 '24 07:07 AayushSabharwal

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.

ChrisRackauckas avatar Jul 03 '24 07:07 ChrisRackauckas

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

AayushSabharwal avatar Jul 03 '24 07:07 AayushSabharwal

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 avatar Jul 03 '24 07:07 ChrisRackauckas

@AayushSabharwal did this get addressed?

ChrisRackauckas avatar Jan 09 '25 08:01 ChrisRackauckas

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

AayushSabharwal avatar Jan 09 '25 09:01 AayushSabharwal

https://github.com/SciML/JumpProcesses.jl/pull/469 is merged

AayushSabharwal avatar Jan 09 '25 12:01 AayushSabharwal

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!

isaacsas avatar Jan 09 '25 12:01 isaacsas

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

AayushSabharwal avatar Jan 09 '25 12:01 AayushSabharwal