SciML / SciMLBase.jl

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

feat: indexing rework with new SymbolicIndexingInterface #532

Closed AayushSabharwal closed 7 months ago

oscardssmith commented 8 months ago

Overall, this looks great! I think there might be a little more cleanup possible by getting rid of syms/paramsyms and always using sys, but this already is a substantial clean up.

AayushSabharwal commented 8 months ago

Did not mean to start a new review, I'm not sure how that happened

ChrisRackauckas commented 7 months ago

Needs a rebase?

AayushSabharwal commented 7 months ago

Yep, almost done

AayushSabharwal commented 7 months ago

Rebased

AayushSabharwal commented 7 months ago

Should SciMLBase also export getp and setp?

codecov[bot] commented 7 months ago

Codecov Report

Attention: 258 lines in your changes are missing coverage. Please review.

Comparison is base (9fdb9c8) 25.91% compared to head (bbba9c6) 0.00%.

Files Patch % Lines
src/integrator_interface.jl 0.00% 67 Missing :warning:
src/solutions/solution_interface.jl 0.00% 67 Missing :warning:
src/scimlfunctions.jl 0.00% 42 Missing :warning:
src/problems/problem_interface.jl 0.00% 34 Missing :warning:
src/symbolic_utils.jl 0.00% 24 Missing :warning:
src/solutions/ode_solutions.jl 0.00% 8 Missing :warning:
src/solutions/optimization_solutions.jl 0.00% 6 Missing :warning:
ext/SciMLBaseZygoteExt.jl 0.00% 3 Missing :warning:
src/ensemble/ensemble_solutions.jl 0.00% 3 Missing :warning:
ext/SciMLBaseChainRulesCoreExt.jl 0.00% 2 Missing :warning:
... and 1 more
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hexaeder commented 7 months ago

In my code I make heavy use of odefunction.syms to get the symbols for plotting purposes, find the right indices in the u vector, and so on. Is there a recommended way to get the symbols now? I found odefunction.sys.variables but this again uses some internals that might change in the future...

AayushSabharwal commented 7 months ago

The recommended way is to import SymbolicIndexingInterface and use variable_symbols(odefunction)

ChrisRackauckas commented 7 months ago

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.

hexaeder commented 7 months ago

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...

ChrisRackauckas commented 7 months ago

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, .syms served us well from 2016-2022, so it did its job 😅. I couldn't have predicted we needed an interface back when it was put in. But now we are in a different place and really formalizing what's going on is the only way for people to make use of all of its functionality correctly and in a way that allows maximum performance.

Datseris commented 7 months ago

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.

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., set_parameter or continuation. Now I'll start learning and doing it the proper way!