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

simplify_fractions removes metadata #434

Closed
hexaeder opened this issue Feb 16, 2022 · 4 comments
Closed

simplify_fractions removes metadata #434

hexaeder opened this issue Feb 16, 2022 · 4 comments

Comments

@hexaeder
Copy link

hexaeder commented Feb 16, 2022

Simplification of x(t)/p where x(t) is a variable does not change the term but removes metadata from x(t).

import Pkg; Pkg.activate(temp=true); Pkg.add("ModelingToolkit")
using ModelingToolkit

@parameters t p
@variables x(t)

term1 = x/p
term2 = simplify(term1)

for v in get_variables(term1)
    println("\n $v => ", getproperty(v, :metadata))
end

for v in get_variables(term2)
    println("\n $v => ", getproperty(v, :metadata))
end

in term2 the x(t) has not metadata anymore!

This was introduced in ModelingToolkit 8.

@hexaeder
Copy link
Author

Seems like the simplify_fractions passes down similarterm to the Walker from MetaTheory since #422. However in the walker the metadata keyword won't get passed (here and here). A recent similar bug was #283 fixed by #284.

cc @0x0f0f0f because it might has to do more with MetaTheory.

@hexaeder hexaeder changed the title Simplification of quotient removes metadata simplify_fractions removes metadata Feb 17, 2022
@0x0f0f0f
Copy link
Member

Seems like the simplify_fractions passes down similarterm to the Walker from MetaTheory since #422. However in the walker the metadata keyword won't get passed (here and here). A recent similar bug was #283 fixed by #284.

cc @0x0f0f0f because it might has to do more with MetaTheory.

Has to do with the design of the walker. Let's see if the walker breaks when passing metatadata down.

@0x0f0f0f
Copy link
Member

0x0f0f0f commented Mar 7, 2022

I think we should merge #436 first, but i'll branch symbolics.jl to see if there's any major breakage (likely).

@hexaeder
Copy link
Author

hexaeder commented Sep 4, 2024

MWE is fixed

@hexaeder hexaeder closed this as completed Sep 4, 2024
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

2 participants