feat: add all_solvable_symbols and all_symbols
#18
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
===========================================
+ Coverage 38.77% 50.96% +12.18%
===========================================
Files 5 5
Lines 98 104 +6
===========================================
+ Hits 38 53 +15
+ Misses 60 51 -9 ☔ View full report in Codecov by Sentry. |
|
What's the next steps here? |
AayushSabharwal
force-pushed
the
as/all-symbols
branch
from
f453494 to
2107adc
Compare
|
Merging |
src/interface.jl
Outdated
| @@ -110,3 +110,19 @@ Check if `sys` has a constant structure. Constant structure systems do not chang | |||
| number of variables or parameters over time. | |||
| """ | |||
| constant_structure(sys) = constant_structure(symbolic_container(sys)) | |||
|
|
|||
| """ | |||
| all_solvable_symbols(sys) | |||
There was a problem hiding this comment.
This name isn't very informative. Also, I think we want it to be a singleton? sol[allvariables], sol[allsymbols]? That lowers to these functions.
And for the API, the user needs to know what the symbol the value corresponds to, since they aren't giving it to you. I would think that this would need to return Pairs between sym => value since otherwise the ordering is simply undefined.
There was a problem hiding this comment.
Sure. I'll address this after fixing the RAT bugs in other repositories
There was a problem hiding this comment.
One thing to consider: allvariables will index variables and observed quantities, which I guess should be fine. allsymbols will also include parameters, which for e.g. solutions will return a Vector{any} since sol[x] is an array but sol[p] is a single value.
I'm also documenting the singletons as returning all variables/symbols at the last timestep so for future implementation of dynamic states the interface is clear.
There was a problem hiding this comment.
Or, wait, did you mean that sol[allvariables] should return a Vector{Pair}? In which case should it be the initial value of each variable?
There was a problem hiding this comment.
Yes, I think so. I don't see any other way for it to be very useful. Or we should document how to easily get that pair.
There was a problem hiding this comment.
Got it. A separate allparameters?
There was a problem hiding this comment.
Yep. And no allsymbols? Or do we keep it for the independent variables?
There was a problem hiding this comment.
No wait, we don't need allparameters either 😅 . We already have parameter_values(sys) which returns the parameter vector. setp support requires that the returned vector be a mutable reference to the actual parameter vector, so mutating it will mutate the parameters. getp(sys, allparameters)(sol) is just parameter_values(sol). setp(sys, allparameters)(sol, new_value) is just parameter_values(sol) .= new_value
There was a problem hiding this comment.
Anything like this is something that should be described in tutorials. Make a small MTK model and show all of the tricks.
|
Note that docs won't render properly until RAT and MTK get merged |
| This also works for arrays or tuples of variables, including observed quantities and | ||
| independent variables: | ||
| ```@example Usage | ||
| sol[[x, y]] |
There was a problem hiding this comment.
BTW, is this using the optimized call now, i.e. a single call to the observed function?
There was a problem hiding this comment.
No, it's broadcasted over the array of symbols, so each one is computed individually. A single call to observed behaves differently:
julia> SymbolicIndexingInterface.observed(sol, [x, y])(sol.u, sol.prob.p, sol.t)
2-element Vector{Vector{Float64}}:
[1.6563910957343146, 0.13029732534611174, 0.0008464888325952026, 1.023645017393648]
[1.9960454103704428, 0.0014132521265988688, 9.986094350108851e-8, 1.0002823510089436]
julia> SymbolicIndexingInterface.observed(sol, (x, y))(sol.u, sol.prob.p, sol.t)
(x(t), y(t))For state variables (whose values are in sol.u) I'd imagine broadcasting is faster than the observed method? It avoids a generated function.
There was a problem hiding this comment.
For state variables it's faster, then for observed it's faster to clump them up. Optimizing this a bit is probably a good idea in the future.
| ```@example Usage | ||
| parameter_values(prob) .= [29.0, 11.0, 2.5] | ||
| parameter_values(prob) | ||
| ``` |
There was a problem hiding this comment.
We should also mention symbolic interpolation indexing, i.e. sol(t; idxs=x), plot indexing plot(sol, idxs=(x,y))
|
Looking good! Let's do the merges then loop back for the docs. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.