feat: indexing rework with new SymbolicIndexingInterface #532
Conversation
| return [xi[s] for xi in x] | ||
| end | ||
|
|
||
| Base.@propagate_inbounds function Base.getindex(x::AbstractEnsembleSolution, |
There was a problem hiding this comment.
I am very happy you were able to get rid of this.
src/integrator_interface.jl
Outdated
| function SymbolicIndexingInterface.is_variable(A::DEIntegrator, sym) | ||
| is_variable(A.f, sym) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.variable_index(A::DEIntegrator, sym) | ||
| variable_index(A.f, sym) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.variable_symbols(A::DEIntegrator) | ||
| variable_symbols(A.f) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.is_parameter(A::DEIntegrator, sym) | ||
| is_parameter(A.f, sym) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.parameter_index(A::DEIntegrator, sym) | ||
| parameter_index(A.f, sym) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.is_independent_variable(A::DEIntegrator, sym) | ||
| is_independent_variable(A.f, sym) | ||
| end | ||
|
|
||
| function SymbolicIndexingInterface.is_observed(A::DEIntegrator, sym) | ||
| return !is_variable(A, sym) && !is_parameter(A, sym) && !is_independent_variable(A, sym) && symbolic_type(sym) == ScalarSymbolic() |
There was a problem hiding this comment.
This doesn't seem ideal. It seems like there should possibly be an easier way to define all of these.
There was a problem hiding this comment.
Off the top of my head, I guess SII could have a function symbolic_indexing_fallback(sys) which specifies what to fallback the methods to. All the other methods then have a default implementation along the lines of is_variable(symbolic_indexing_fallback(sys), sym)
src/scimlfunctions.jl
Outdated
|
|
||
| function SymbolicIndexingInterface.is_variable(fn::AbstractSciMLFunction, sym) | ||
| has_sys(fn) && return is_variable(fn.sys, sym) | ||
| has_syms(fn) && return Symbol(sym) in fn.syms |
There was a problem hiding this comment.
Any chance we can get rid of this part? i.e. in the SciMLFunction constructor we could turn syms/paramsyms into a sys with the appropriate behavior.
| if has_sys(sol.prob.f) | ||
| DiffEqArray{typeof(A).parameters[1:4]..., typeof(sol.prob.f.sys), typeof(observed), | ||
| typeof(p)}(A.u, | ||
| A.t, | ||
| sol.prob.f.sys, | ||
| observed, | ||
| p) | ||
| else | ||
| syms = hasproperty(sol.prob.f, :syms) && sol.prob.f.syms !== nothing ? | ||
| [sol.prob.f.syms[idxs]] : nothing | ||
| DiffEqArray(A.u, A.t, syms, getindepsym(sol), observed, p) | ||
| end | ||
| return DiffEqArray(A.u, A.t, p, sol) |
There was a problem hiding this comment.
this is happy-making :)
There was a problem hiding this comment.
My favourite part of this PR was removing all the getindex methods for AbstractTimeseriesSolution.
|
Overall, this looks great! I think there might be a little more cleanup possible by getting rid of |
|
Did not mean to start a new review, I'm not sure how that happened |
AayushSabharwal
force-pushed
the
as/indexing-rework
branch
from
97d0919 to
3bbdcc2
Compare
AayushSabharwal
force-pushed
the
as/indexing-rework
branch
from
cd2a0a8 to
9ca3739
Compare
|
Needs a rebase? |
- Implement SII for solutions, problems, SciMLFunctions, integrators - Solution indexing falls back to AbstractVectorOfArray indexing - remove issymbollike - remove has_static_variable - add new parameter indexing - fix adjoint definitions - Remove syms, paramsyms, indepsym from SciMLFunctions, use SymbolCache in place of f.sys instead
|
Yep, almost done |
AayushSabharwal
force-pushed
the
as/indexing-rework
branch
from
5603821 to
bbba9c6
Compare
|
Rebased |
|
Should SciMLBase also export |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
==========================================
- Coverage 25.91% 0.00% -25.92%
==========================================
Files 53 53
Lines 4140 4034 -106
==========================================
- Hits 1073 0 -1073
- Misses 3067 4034 +967 ☔ View full report in Codecov by Sentry. |
|
In my code I make heavy use of |
|
The recommended way is to import |
|
Indeed. The main purpose of this transition is to establish a well-defined interface that this is all based off of, rather than having to know internals voodoo for what's setup related to symbolic indexing. This is https://github.com/SciML/SymbolicIndexingInterface.jl, with our downstream packages implemented the documented interface https://docs.sciml.ai/SymbolicIndexingInterface/stable/api/ (and tutorials coming soon). If you stick to the documented interface and version control with respect to that, you will get guarantees of correctness in the setup which relying on internals could never give you. Hopefully this makes everything easier to maintain in the future, not just for us but for anyone like you who downstream is trying to use/extend the symbolic indexing in any way. |
|
Thanks a lot for the details, I'll look into that package! Pleas don't get me wrong, I did not mean to complain about the "breakage" at all -- that's what you get for using internals. I really like the transition to more and more tested and guaranteed interfaces. Luckily we have funding for a major rewrite next your so we can base our package on modern SciML standards removing many of those 3 year old hacks... |
|
Yes no worries. Interfaces are great, but you definitely have to rip off a few bandaids to put one in place where there wasn't one before. I mean, |
Great, thanks for the work @AayushSabharwal and @ChrisRackauckas and everyone else working on this. I was just about to interface ODESystem in DynamicalSystems.jl and allow using symbols for e.g., |
No description provided.