fix: change parameter_values(::GraphSystemParameters) to access params_partitioned

[vyudu]

I ran into an issue where it would stack overflow whenever I try to setp on a GraphSystemParameters object, because set_parameter!(sys, val, idx) is implemented as set_parameter!(parameter_values(sys), val, idx) in SII, but parameter_values(::GraphSystemParameters) just returns itself. So this change fixes that recursion. But I am not sure if the semantics of parameter_values remains correct in this case.

[MasonProtter]

Hm, no I don't think this is correct since the params_partitioned are not the only parameters, there's also all the other stuff in the GraphSystemParameters like the connection matrices.

We might have to add an additional dispatch somewhere to avoid the stack overflow

[vyudu]

Like this? Doesn't handle the connections but will people be using setp for those?

More generally I wanted to ask what the idiomatic thing to do is when you need to change the parameters of a problem. Is it just to setp on prob.p, and then remake the problem with the new p object?

[MasonProtter]

Yeah, we do use setp for connections as well.

More generally I wanted to ask what the idiomatic thing to do is when you need to change the parameters of a problem. Is it just to setp on prob.p, and then remake the problem with the new p object?

Not really, I would just setp on the prob itself rather than prob.p, I never tried to use setp on the parameter object itself, I guess that's why I never ran into this problem. I think you're not really ever suppopsed to use setp on a parameter object because they don't usually contain the namemapping. But our parameter object does contain the namemap, so I guess it's not the craziest thing to do. It does seem a little questionable though

[vyudu]

I think there's an issue when using setp on a prob in a way that changes the type of some parameter. Since it creates a copy of the GraphSystemParameters any time there's a parameter modification with a type change, prob.p doesn't actually get mutated... we would need to @reset prob.p = setparams!!... somewhere but this seems like it would require pirating set_parameter! for ODEProblem?

EDIT: hm this happens if you try to directly setp the parameter object too.

[MasonProtter]

Oh, does that not currently error? We should be throwing an error when the user tries to change the type with setp. setp is only okay for type stable changes. E.g. here's how it works with MTK:

julia> using ModelingToolkit

julia> using ModelingToolkit: t_nounits as t, D_nounits as D

julia> let
           ps = @parameters a=1.0
           vs = @variables x(t)=1.0
           eqs = [D(x) ~ a*x]
           @named sys = System(eqs, t)
           prob = ODEProblem(structural_simplify(sys), [], (0.0, 1.0), [])
           setp(prob, a)(prob, 2.0 + im)
       end
ERROR: InexactError: Float64(2.0 + 1.0im)
Stacktrace:
 [1] Real
   @ ./complex.jl:44 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] setindex!
   @ ./array.jl:985 [inlined]
 [4] set_parameter!
   @ ~/.julia/packages/ModelingToolkit/Aomff/src/systems/parameter_buffer.jl:364 [inlined]
 [5] set_parameter!
   @ ~/.julia/packages/SymbolicIndexingInterface/sZ3a9/src/value_provider_interface.jl:67 [inlined]
 [6] SetParameterIndex
   @ ~/.julia/packages/SymbolicIndexingInterface/sZ3a9/src/parameter_indexing.jl:699 [inlined]
 [7] (::SymbolicIndexingInterface.ParameterHookWrapper{…})(prob::ODEProblem{…}, args::ComplexF64)
   @ SymbolicIndexingInterface ~/.julia/packages/SymbolicIndexingInterface/sZ3a9/src/parameter_indexing.jl:667
 [8] top-level scope
   @ REPL[24]:7
Some type information was truncated. Use `show(err)` to see complete types.

[vyudu]

Ah OK. Yeah it just silently doesn't change it at the moment, I can add that to this PR. What's the way to change a parameter type then?

[MasonProtter]

remake is necessary if types are changing

[vyudu]

What about type promotion? MTKParameters can promote like 2 to a Float64 because it's an array but is there a way to do that with our SubsystemParameters?

[vyudu]

This bit is to allow values to be iterators because previously it was trying to splat them and that was erroring

[MasonProtter]

We'll want to support stuff like setting an Int where the value is actually a Float64, but it's a little complicated. Lets do that in a followup PR and just get this merged first.