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 order #1182

Closed
schillic opened this issue Jun 27, 2024 · 6 comments · Fixed by #1219
Closed

get_variables order #1182

schillic opened this issue Jun 27, 2024 · 6 comments · Fixed by #1219

Comments

@schillic
Copy link

A recent change about sorting in SymbolicUtils changed the behavior of get_variables here. Is there a contract about the order in which the variables are returned? The documentation does not say anything about that, but I have the impression that they were sorted before, so for me this was a breaking change. More confusingly, the function now returns a different result when receiving a parsed expression object or an expression for parsing:

Old behavior
pkg> add SymbolicUtils@2.0

julia> using Symbolics

julia> vars = @variables x y
2-element Vector{Num}:
 x
 y

julia> Symbolics.get_variables(x - y)
2-element Vector{Any}:
 x
 y

julia> expr = x - y
x - y

julia> Symbolics.get_variables(expr)
2-element Vector{Any}:
 x
 y
New behavior
pkg> add SymbolicUtils@2.1, Symbolics@5.31

julia> using Symbolics

julia> vars = @variables x y
2-element Vector{Num}:
 x
 y

julia> Symbolics.get_variables(x - y)  # order swapped here
2-element Vector{Any}:
 y
 x

julia> expr = x - y
x - y

julia> Symbolics.get_variables(expr)  # order not swapped here
2-element Vector{Any}:
 x
 y

Could you please confirm that this is expected behavior, and clarify how I can obtain a consistent order of the variables?

@ChrisRackauckas
Copy link
Member

Yes, there was no guarantee on the sorting order, and a recent change removes a previously enforced sorting which greatly improves performance. @bowenszhu is there a sorted version of this function?

@bowenszhu
Copy link
Member

Symbolics.get_variables doesn't offer a built-in way to obtain a sorted list of variables (as of Symbolics v5.32.0).

@ChrisRackauckas Currently, we have arguments and sorted_arguments. To enhance flexibility, how about we remove sorted_arguments, modify arguments(x) to accept a sort keyword argument arguments(x; sort = false) and add a kwargs... argument to relevant function signatures? This allows users to explicitly control sorting behavior.

@ChrisRackauckas
Copy link
Member

I think that was being discussed with @0x0f0f0f ? I think that's a good idea.

@0x0f0f0f
Copy link
Member

@bowenszhu Can't users already control this? We discussed (It took us 3 years) to avoid keyword arguments because they make implementing TermInterface.jl unstraightforward. It would be a bit of a PITA to change TermInterface again and go through another major release cycle. This is expected and I've warned about this in JuliaSymbolics/SymbolicUtils.jl#609

What about we just change get_variables to use sorted_arguments? Was there any guarantee in what order should get_variables have returned the variables? In order of appearance? Lexicographically ordered?

I guess the latter, with <\_e would be ok

@schillic
Copy link
Author

A change in get_variables, either by default or via an additional argument, would be just fine. Or really any other way to get variables sorted (lexicographically or order of definition should be sufficient).

I am a bit surprised that nobody else seems to be needing this feature. We use Symbolics to parse expressions like 2x + 3y == 5 and to extract the a and b from the pattern a·z == b. Previously I got the expected a = [2, 3], but with the new version, this now results in a = [3, 2], which makes it not usable anymore. Maybe using Symbolics for that is overkill, but it used to work nicely.

@ChrisRackauckas
Copy link
Member

Can't we just do sorted_get_variables which just uses sorted_arguments and this is done?

@bowenszhu bowenszhu linked a pull request Aug 14, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

4 participants