SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
141 stars 101 forks source link

SII based plot recipe now exposes internal, non-user variables #763

Open isaacsas opened 3 months ago

isaacsas commented 3 months ago

Sometime after 9.10 the plot recipe has started to include propensity variables within plots, i.e. compare

https://docs.sciml.ai/JumpProcesses/v9.12/tutorials/discrete_stochastic_example/c355f569.svg

vs. the 9.10.1 version:

https://docs.sciml.ai/JumpProcesses/v9.10/tutorials/discrete_stochastic_example/86922c19.svg

I assume this is a plot-recipe change in SciMLBase, but don't know how / why it was ignoring them before.

ChrisRackauckas commented 3 months ago

I didn't know it dropped some before. The current version is what we'd considered the correct one though?

isaacsas commented 3 months ago

I thought the goal was to hide the ExtendedJumpArray aspect from users and make solution objects look as much like ODE solutions as possible?

ChrisRackauckas commented 3 months ago

oh true. Yeah I see, that's why they were hidden 😅. I'm not sure why that would have changed with a release. That's pretty hard to test too. I may have to dig in.

isaacsas commented 3 months ago

FWIW, the last version that didn't have this behavior within a release was on SciMLBase 2.10. We then jumped with 9.11. to SciMLBase 2.31 where it appears. For JumpProcesses the change occurred with 9.11's release, but on quick glance I see nothing in the merged PRs that could cause this within JumpProcesses.

isaacsas commented 3 months ago

OK, I've verified the issue appears with SciMLBase 2.12 which had only this PR:

https://github.com/SciML/SciMLBase.jl/pull/572

which updated the plot recipe to use SII. So it seems that changed the indexing in a way that the propensity values are now being plotted.

Here is a MWE if it would be useful for testing:

 using JumpProcesses, OrdinaryDiffEq, Plots
rate(u,p,t) = (1+t)*u[1]
affect(integrator) = (integrator.u[1] -= 1; nothing)
vrj = VariableRateJump(rate,affect)
oprob = ODEProblem((du,u,p,t) -> (du .=0; nothing), [10.0], (0.0, 10.0))
jprob = JumpProblem(oprob, Direct(), vrj)
sol = solve(jprob, Tsit5())
plot(sol)

Should I move this issue to SciMLBase?

ChrisRackauckas commented 3 months ago

Yeah. The plot recipe does not have a real interface to opt variables in/out by default. It worked mostly because of a quirk of indexing and size checking that we probably shouldn't be relying on. It needs to be made into a real documented and tested interface.

isaacsas commented 3 months ago

I transferred this to SciMLBase, since my understanding is this requires changes in how the plot recipe works and isn't something we can handle at the level of JumpProcesses currently.

isaacsas commented 3 months ago

It does seem like it would be nice to have an interface to separate internal solver variables from user variables that could be used by both solution evaluation and plot recipes. Maybe it only impacts JumpProcesses via the ExtendedJumpArrays right now, but I'd imagine there are other use cases where solvers might want to internally augment user-variables with additional variables, but then not expose those back to users (or at least not expose them in default views -- having them available in a more advanced interface still makes sense).