Skip to content
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

get_variables drops metadata for callables #1396

Closed
SebastianM-C opened this issue Jan 6, 2025 · 3 comments
Closed

get_variables drops metadata for callables #1396

SebastianM-C opened this issue Jan 6, 2025 · 3 comments

Comments

@SebastianM-C
Copy link
Contributor

SebastianM-C commented Jan 6, 2025

It looks like there are some cases in which get_variables drops variable metadata, leading to the variables that are return to not be isequal to the original ones. This seems to be the case for callables, which are currently generated by the @mtkmodel macro as pointed out in #1364 (comment).

MWE

using Symbolics

@variables t x(..)
x = x(t)

Symbolics.get_variables(x + 1)
# 2-element Vector{SymbolicUtils.BasicSymbolic}:
# x
# t

isequal(Symbolics.get_variables(x + 1)[1], Symbolics.value(x)) # false

Symbolics.metadata(Symbolics.value(x))
# Base.ImmutableDict{DataType, Any} with 2 entries:
#   CallWithParent => x⋆
#   VariableSource => (:variables, :x)

Symbolics.metadata(Symbolics.get_variables(x + 1)[1]) # nothing

While SciML/ModelingToolkit.jl#3217 could help avoid the issue for @mtkmodel based systems, I would expect the metadata to still be there.

@ChrisRackauckas
Copy link
Member

@AayushSabharwal might this be related to one of your changes?

@AayushSabharwal
Copy link
Contributor

Yeah so #1361 made get_variables search inside x(t) IF x is declared as @variables x(..). Previously, get_variables would return x(t) and now it returns x(..) and t. The problem is that when x(..) is called, the metadata is attached to the called object x(t) and not to the function x(..). The consensus from #1364 was that get_variables should be reverted and the MTK functions should get a kwarg for the search function to use. I'll make this change

@AayushSabharwal
Copy link
Contributor

This can be closed via SciML/ModelingToolkit.jl#3217 instead of #1364.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants