SciML / SymbolicIndexingInterface.jl

A general interface for symbolic indexing of SciML objects used in conjunction with Domain-Specific Languages
https://docs.sciml.ai/SymbolicIndexingInterface/stable/
MIT License
14 stars 6 forks source link

`StackOverflow` on `is_timeseries_parameter` #85

Closed hexaeder closed 3 weeks ago

hexaeder commented 2 months ago

Describe the bug 🐞

According to the docs, you should define

symbolic_container(indp::MyIndexProvier) = indp

in case MyIndexProvider implements the interface:

https://github.com/SciML/SymbolicIndexingInterface.jl/blob/e7dd822c3dd5aa60161f77e6e96a32ce5c4941bb/src/index_provider_interface.jl#L2-L7

However, this leads to a stack overflow in the default implementation of is_timeseries_parameter

https://github.com/SciML/SymbolicIndexingInterface.jl/blob/e7dd822c3dd5aa60161f77e6e96a32ce5c4941bb/src/index_provider_interface.jl#L61-L67

as it descents forever as long as the symbolic_container method exists.

The docs do not mention any new methods to implement to fulfill the index provider interface.

hexaeder commented 1 month ago

Same problem seems to apply to appear with get_all_timeseries_indexes now to.

AayushSabharwal commented 1 month ago

Thanks for pointing this out, #90 should fix this

hexaeder commented 3 weeks ago

Thanks for the fix, unfortunately the problem still pops up, now in a method in SciMLBase.

https://github.com/SciML/SciMLBase.jl/blob/56ba8819c1637fffbae5722057c5532b8d48c21c/src/solutions/ode_solutions.jl#L360-L366

However I removed the symbolic_container(inpr::MyIndPr) = inpr definition and everything it seems to work. Maybe this should be reflected in the symbolic_container docstring, that the method should not be implemented for types implementing the interface...

https://github.com/SciML/SymbolicIndexingInterface.jl/blob/58e0df3acc973ac31e9d040a4cd8b1e2ad0056db/src/index_provider_interface.jl#L4-L5

AayushSabharwal commented 3 weeks ago

Yeah that does seem like the better approach. It is technically breaking, but we should be able to get away with it.