-
-
Notifications
You must be signed in to change notification settings - Fork 233
fix: some fixes related to usage of array symbolics #3126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: some fixes related to usage of array symbolics #3126
Conversation
74d69d8 to
3d743bc
Compare
|
Requires JuliaSymbolics/Symbolics.jl#1309 |
697048a to
d1f61f1
Compare
|
This should be good to go now |
|
It looks like this causes a lot of issues to MethodOfLines? |
|
Yeah, it seems the CSE hack is invalid in some cases. I'll work on fixing it. |
2d72d68 to
ed9ec6d
Compare
| # ideally, we want to support equations such as `p ~ [p[1], p[2]]` which will then be handled | ||
| # by the topological sorting and dependency identification pieces | ||
| obs_arr_subs = Dict() | ||
| unknowns = Any[v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to refactor this into a function call that is able to be turned on/off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll pull it out into a function and propagate a keyword argument to toggle it. Should it be on by default or off? I feel like HACK2 should be on by default, and CSE off since that's the more bug-prone of the two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSE one seems pretty essential to a lot of applications? I would try to see if we can get them on by default. But yes making an option would then make it much easier to isolate any potential bugs to it.
64eddbc to
d2fe0eb
Compare
|
There's a bug |
ec1a21a to
1025363
Compare
Requires JuliaSymbolics/Symbolics.jl#1307
Regarding the improvement of the "hack", the issue with the old implementation was that it put
OffsetArrays inside equations, which we can't codegen (SymbolicUtils.Code.create_array(::Type{<:OffsetArray}, ...)doesn't exist). This fixes that so we generate code equivalent toy = OffsetArrays.Origin(0)([1, 2, 3]).Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.