Optimize `arguments` function by removing sorting #615

bowenszhu · 2024-06-16T20:39:00Z

The arguments function currently sorts the arguments of a BasicSymbolic expression, which is expensive and often unnecessary. This change removes the unnecessary sorting, resulting in improved performance and reduced memory usage.

Sorting arguments remains for printing and Expr generation.

A test case is skipped because the arguments is not sorted in the construction step of PolyForm which leads to unpredictable printing output.

SymbolicUtils.jl/src/polyform.jl

Line 106 in 43797eb

args = arguments(x)

github-actions · 2024-06-16T20:47:08Z

Benchmark Results

	master	`274e3cf`...	master/274e3cfe5280e8...
overhead/acrule/a+2	0.747 ± 0.014 μs	0.718 ± 0.022 μs	1.04
overhead/acrule/a+2+b	0.732 ± 0.017 μs	0.683 ± 0.02 μs	1.07
overhead/acrule/a+b	0.253 ± 0.0099 μs	0.25 ± 0.0084 μs	1.01
overhead/acrule/noop:Int	25.6 ± 0.92 ns	25.6 ± 0.05 ns	1
overhead/acrule/noop:Sym	0.0367 ± 0.0058 μs	0.0367 ± 0.0051 μs	1
overhead/rule/noop:Int	0.0437 ± 0.0012 μs	0.0442 ± 0.00091 μs	0.987
overhead/rule/noop:Sym	0.0554 ± 0.0029 μs	0.0575 ± 0.0028 μs	0.962
overhead/rule/noop:Term	0.0565 ± 0.0028 μs	0.0579 ± 0.0029 μs	0.976
overhead/ruleset/noop:Int	0.13 ± 0.004 μs	0.139 ± 0.0046 μs	0.935
overhead/ruleset/noop:Sym	0.152 ± 0.0047 μs	0.163 ± 0.0069 μs	0.93
overhead/ruleset/noop:Term	3.84 ± 0.16 μs	4.13 ± 0.19 μs	0.93
overhead/simplify/noop:Int	0.151 ± 0.0033 μs	0.164 ± 0.0016 μs	0.921
overhead/simplify/noop:Sym	0.166 ± 0.0043 μs	0.17 ± 0.0039 μs	0.975
overhead/simplify/noop:Term	0.0416 ± 0.002 ms	0.0399 ± 0.002 ms	1.04
overhead/simplify/randterm (+, *):serial	0.129 ± 0.0065 s	0.09 ± 0.0028 s	1.43
overhead/simplify/randterm (+, *):thread	0.0737 ± 0.025 s	0.0543 ± 0.031 s	1.36
overhead/simplify/randterm (/, *):serial	0.239 ± 0.0076 ms	0.229 ± 0.0088 ms	1.04
overhead/simplify/randterm (/, *):thread	0.267 ± 0.007 ms	0.266 ± 0.0091 ms	1
overhead/substitute/a	0.058 ± 0.0018 ms	0.0596 ± 0.0014 ms	0.973
overhead/substitute/a,b	0.0512 ± 0.0017 ms	0.0518 ± 0.0014 ms	0.988
overhead/substitute/a,b,c	17.1 ± 0.85 μs	17.1 ± 0.76 μs	0.999
polyform/easy_iszero	0.0355 ± 0.0021 ms	0.0365 ± 0.0018 ms	0.973
polyform/isone	3.1 ± 0 ns	3.1 ± 0.009 ns	1
polyform/iszero	1.89 ± 0.045 ms	1.3 ± 0.038 ms	1.45
polyform/simplify_fractions	2.59 ± 0.062 ms	1.91 ± 0.059 ms	1.36
time_to_load	4.45 ± 0.17 s	4.5 ± 0.064 s	0.988

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

because the order of arguments is not sorted in the construction step leading to unpredictable output https://github.com/JuliaSymbolics/SymbolicUtils.jl/blob/43797eb43b33b8d6b0374eac8cd6f87f94bcf79d/src/polyform.jl#L106

YingboMa

The MTK failure looks real. Could you check if latex ouput has been properly sorted?

bowenszhu · 2024-06-20T08:52:38Z

The MTK failure looks real. Could you check if latex ouput has been properly sorted?

Thanks.
Resolved in Symbolics.jl

Refactor: Migrate from deprecated unsorted_arguments to arguments Symbolics.jl#1179

ChrisRackauckas · 2024-06-20T11:09:37Z

src/inspect.jl

 function AbstractTrees.children(x::Symbolic)
-    iscall(x) ? arguments(x) : isexpr(x) ? children(x) : ()
+    iscall(x) ? arguments(x; sort = true) : isexpr(x) ? children(x) : ()


see JuliaSymbolics/TermInterface.jl#44 (comment)

Shouldn't this not force sort then, given that would be sorted_children?

Which option do you prefer?

Adding a sort keyword argument.

Creating new functions sorted_arguments and sorted_children.

Also, the arguments and children functions in SymbolicUtils.jl v2.0.2 aren't related to TermInterface.jl yet.

Let's get the TermInterface.jl change in and hit that interface. Since that's happening like tomorrow there's no need to do a separate version as well.

Which option do you prefer?

1. Adding a `sort` keyword argument. 2. Creating new functions `sorted_arguments` and `sorted_children`.

Also, the arguments and children functions in SymbolicUtils.jl v2.0.2 aren't related to TermInterface.jl yet.

@bowenszhu TermInterface.jl 2.0 was merged with sorted_argument and sorted_children. I have a PR open for TermInterface.jl 1.0, which should be updated to 2.0 - want update this MR to target that branch? I'll be at a conference and won't have much time.

ChrisRackauckas · 2024-06-23T15:54:21Z

This should bump to TermInterface v2?

0x0f0f0f · 2024-06-23T15:56:46Z

This should bump to TermInterface v2?

@ChrisRackauckas there's already #609 (comment) open - which is most of the work. Diff between TI 1.0 and 2.0 is just introduction to sorted_children and sorted_arguments.

bowenszhu · 2024-06-23T17:06:13Z

This pull request is separate from TermInferface. We could integrate this with TermInferface in a future PR.

ChrisRackauckas · 2024-06-23T19:04:37Z

Interestingly, the downstream tests say this is pretty breaking.

bowenszhu · 2024-06-23T21:14:10Z

This SymbolicUtils.jl pull request removed the function SymbolicUtils.unsorted_arguments. This function is currently being used in ModelingToolkit.jl, and its removal causes compilation errors.
https://github.com/search?q=repo%3ASciML%2FModelingToolkit.jl%20unsorted_arguments&type=code

ChrisRackauckas · 2024-06-23T21:48:43Z

Just add it back and have it forward to arguments with a deprecation

ChrisRackauckas · 2024-06-24T00:59:47Z

Still some downstream failures

0x0f0f0f · 2024-06-24T11:58:16Z

This pull request is separate from TermInferface. We could integrate this with TermInferface in a future PR.

That's duplicating work. I would just update #609

0x0f0f0f · 2024-06-24T12:26:30Z

It would be amazing if you could target #609

…ents-to-unsorted_arguments-to-accelerate-term-traversal

bowenszhu · 2024-06-25T05:09:42Z

This pull request itself is non-breaking.

This pull request introduces changes that result in test failures within the CI, specifically due to deprecation warnings for unsorted_arguments and the change from arguments to sorted_arguments:

Symbolics Integration Tests: All 4 failed test cases are addressed in a related PR: Refactor: Migrate from deprecated unsorted_arguments to arguments Symbolics.jl#1179.
ModelingToolkit Integration Tests: All 4 failed test cases are because @test_nowarn and @test_logs assertions are now catching the deprecation warnings, which is being resolved in PR Refactor: Migrate from deprecated unsorted_arguments to arguments SciML/ModelingToolkit.jl#2816.

ChrisRackauckas · 2024-06-25T09:14:19Z

That's duplicating work. I would just update #609

That PR has breaking updates, this PR is a non-breaking update. So we should merge this and then rebase the other changes on top of this

bowenszhu linked an issue Jun 16, 2024 that may be closed by this pull request

Default arguments to unsorted_arguments to accelerate term traversal #613

Closed

Add optional sorting to arguments

678bce8

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch from e90b702 to 678bce8 Compare June 17, 2024 05:32

bowenszhu added 5 commits June 17, 2024 00:08

Sort arguments in get_degrees for printing

6899d5f

Remove unnecessary variable assignment in get_degrees

e8d04c1

Return sorted arguments in AbstractTrees.children for printing

df8f204

Change sort from optional to keyword argument in arguments

9a56817

Skip a test case of PolyForm printing

890964d

because the order of arguments is not sorted in the construction step leading to unpredictable output https://github.com/JuliaSymbolics/SymbolicUtils.jl/blob/43797eb43b33b8d6b0374eac8cd6f87f94bcf79d/src/polyform.jl#L106

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch from 4ea0338 to 0f56a04 Compare June 17, 2024 19:39

Sort arguments in function_to_expr

7b2c945

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch from 0f56a04 to 7b2c945 Compare June 17, 2024 21:00

bowenszhu marked this pull request as ready for review June 17, 2024 21:53

bowenszhu requested review from YingboMa and ChrisRackauckas June 19, 2024 12:50

YingboMa reviewed Jun 20, 2024

View reviewed changes

bowenszhu mentioned this pull request Jun 20, 2024

Keep sorting arguments for Latexify JuliaSymbolics/Symbolics.jl#1172

Closed

ChrisRackauckas reviewed Jun 20, 2024

View reviewed changes

bowenszhu added 5 commits June 20, 2024 09:23

children keeps the same default behavior with arguments

fe7ed96

Sort children for inspection via AbstractTrees

93a8f17

Adjust interface, add sorted_* functions for arguments & children

4e26a2d

Change all unsorted_arguments to arguments

590083e

Change (un)sorted_arguments import & export

bcb7c1b

ChrisRackauckas mentioned this pull request Jun 23, 2024

TermInterface Version 2 #609

Merged

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch from 14052d7 to 125506f Compare June 23, 2024 23:08

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch 2 times, most recently from 5050b6d to 4fda0a8 Compare June 24, 2024 01:23

bowenszhu marked this pull request as draft June 24, 2024 02:18

Deprecate unsorted_arguments in favor of arguments

ae1cbad

bowenszhu force-pushed the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch from 2f135ea to ae1cbad Compare June 24, 2024 05:55

This was referenced Jun 24, 2024

Fix: Resolve head ambiguity and prevent UndefVarError and precompilation failure JuliaSymbolics/Symbolics.jl#1178

Merged

Import maketerm in SymbolicUtils.Code to resolve UndefVarError #619

Merged

Merge remote-tracking branch 'origin/master' into b/613-default-argum…

274e3cf

…ents-to-unsorted_arguments-to-accelerate-term-traversal

bowenszhu mentioned this pull request Jun 25, 2024

Refactor: Migrate from deprecated unsorted_arguments to arguments JuliaSymbolics/Symbolics.jl#1179

Merged

bowenszhu marked this pull request as ready for review June 25, 2024 05:05

bowenszhu mentioned this pull request Jun 25, 2024

Refactor: Migrate from deprecated unsorted_arguments to arguments SciML/ModelingToolkit.jl#2816

Merged

5 tasks

ChrisRackauckas merged commit a69f8e8 into master Jun 25, 2024
8 of 12 checks passed

ChrisRackauckas deleted the b/613-default-arguments-to-unsorted_arguments-to-accelerate-term-traversal branch June 25, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize `arguments` function by removing sorting #615

Optimize `arguments` function by removing sorting #615

bowenszhu commented Jun 16, 2024 •

edited

Loading

github-actions bot commented Jun 16, 2024 •

edited

Loading

YingboMa left a comment

bowenszhu commented Jun 20, 2024 •

edited

Loading

ChrisRackauckas Jun 20, 2024

ChrisRackauckas Jun 21, 2024

bowenszhu Jun 21, 2024

ChrisRackauckas Jun 22, 2024

0x0f0f0f Jun 23, 2024

ChrisRackauckas commented Jun 23, 2024

0x0f0f0f commented Jun 23, 2024

bowenszhu commented Jun 23, 2024

ChrisRackauckas commented Jun 23, 2024

bowenszhu commented Jun 23, 2024

ChrisRackauckas commented Jun 23, 2024

ChrisRackauckas commented Jun 24, 2024

0x0f0f0f commented Jun 24, 2024

0x0f0f0f commented Jun 24, 2024

bowenszhu commented Jun 25, 2024 •

edited

Loading

ChrisRackauckas commented Jun 25, 2024

Optimize arguments function by removing sorting #615

Optimize arguments function by removing sorting #615

Conversation

bowenszhu commented Jun 16, 2024 • edited Loading

github-actions bot commented Jun 16, 2024 • edited Loading

Benchmark Results

Benchmark Plots

YingboMa left a comment

Choose a reason for hiding this comment

bowenszhu commented Jun 20, 2024 • edited Loading

ChrisRackauckas Jun 20, 2024

Choose a reason for hiding this comment

ChrisRackauckas Jun 21, 2024

Choose a reason for hiding this comment

bowenszhu Jun 21, 2024

Choose a reason for hiding this comment

ChrisRackauckas Jun 22, 2024

Choose a reason for hiding this comment

0x0f0f0f Jun 23, 2024

Choose a reason for hiding this comment

ChrisRackauckas commented Jun 23, 2024

0x0f0f0f commented Jun 23, 2024

bowenszhu commented Jun 23, 2024

ChrisRackauckas commented Jun 23, 2024

bowenszhu commented Jun 23, 2024

ChrisRackauckas commented Jun 23, 2024

ChrisRackauckas commented Jun 24, 2024

0x0f0f0f commented Jun 24, 2024

0x0f0f0f commented Jun 24, 2024

bowenszhu commented Jun 25, 2024 • edited Loading

ChrisRackauckas commented Jun 25, 2024

Optimize `arguments` function by removing sorting #615

Optimize `arguments` function by removing sorting #615

bowenszhu commented Jun 16, 2024 •

edited

Loading

github-actions bot commented Jun 16, 2024 •

edited

Loading

bowenszhu commented Jun 20, 2024 •

edited

Loading

bowenszhu commented Jun 25, 2024 •

edited

Loading