TermInterface Version 2

[0x0f0f0f]

No description provided.

[0x0f0f0f]

Have to adjust docs

[github-actions]

Benchmark Results

	master	4284270...	master/42842702af070c...
overhead/acrule/a+2	0.742 ± 0.02 μs	0.734 ± 0.016 μs	1.01
overhead/acrule/a+2+b	0.72 ± 0.021 μs	0.733 ± 0.02 μs	0.982
overhead/acrule/a+b	0.251 ± 0.0086 μs	0.26 ± 0.0083 μs	0.965
overhead/acrule/noop:Int	25.9 ± 0.05 ns	25.9 ± 0.059 ns	1
overhead/acrule/noop:Sym	0.0376 ± 0.005 μs	0.0367 ± 0.0052 μs	1.02
overhead/rule/noop:Int	0.0439 ± 0.00065 μs	0.0444 ± 0.0006 μs	0.989
overhead/rule/noop:Sym	0.0555 ± 0.0031 μs	0.0551 ± 0.0025 μs	1.01
overhead/rule/noop:Term	0.0564 ± 0.0036 μs	0.0545 ± 0.0025 μs	1.03
overhead/ruleset/noop:Int	0.128 ± 0.003 μs	0.127 ± 0.0028 μs	1
overhead/ruleset/noop:Sym	0.151 ± 0.0062 μs	0.16 ± 0.0057 μs	0.949
overhead/ruleset/noop:Term	3.55 ± 0.14 μs	3.22 ± 0.16 μs	1.1
overhead/simplify/noop:Int	0.142 ± 0.002 μs	0.145 ± 0.0034 μs	0.976
overhead/simplify/noop:Sym	0.154 ± 0.0047 μs	0.157 ± 0.0078 μs	0.976
overhead/simplify/noop:Term	0.0392 ± 0.0021 ms	0.0366 ± 0.0018 ms	1.07
overhead/simplify/randterm (+, *):serial	0.0925 ± 0.0023 s	0.0906 ± 0.0013 s	1.02
overhead/simplify/randterm (+, *):thread	0.0533 ± 0.032 s	0.0507 ± 0.033 s	1.05
overhead/simplify/randterm (/, *):serial	0.225 ± 0.0093 ms	0.217 ± 0.0081 ms	1.04
overhead/simplify/randterm (/, *):thread	0.261 ± 0.01 ms	0.246 ± 0.0088 ms	1.06
overhead/substitute/a	0.0552 ± 0.0017 ms	0.0585 ± 0.0015 ms	0.942
overhead/substitute/a,b	0.0499 ± 0.0017 ms	0.0523 ± 0.0019 ms	0.955
overhead/substitute/a,b,c	17.9 ± 0.77 μs	16.8 ± 0.73 μs	1.07
polyform/easy_iszero	0.0319 ± 0.0025 ms	29.9 ± 2.2 μs	1.07
polyform/isone	3.1 ± 0.009 ns	2.79 ± 0.01 ns	1.11
polyform/iszero	1.19 ± 0.032 ms	1.14 ± 0.032 ms	1.05
polyform/simplify_fractions	1.8 ± 0.044 ms	1.64 ± 0.042 ms	1.1
time_to_load	4.56 ± 0.033 s	4.56 ± 0.022 s	1

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).

[YingboMa]

Term manipulation could change symtype. If we don't want maketerm to take symtype, we should call promote_symtype but that's a dynamic dispatch and expensive. Should we make symtype a keyword argument that defaults to promote_symtype?

[0x0f0f0f]

Term manipulation could change symtype. If we don't want maketerm to take symtype, we should call promote_symtype but that's a dynamic dispatch and expensive. Should we make symtype a keyword argument that defaults to promote_symtype?

Mmmmh. This is specific to SymbolicUtils.jl and promote_symtype would not make sense in TermInterface.jl because other dependents do not use it.

[0x0f0f0f]

Term manipulation could change symtype. If we don't want maketerm to take symtype, we should call promote_symtype but that's a dynamic dispatch and expensive. Should we make symtype a keyword argument that defaults to promote_symtype?

Mmmmh. This is specific to SymbolicUtils.jl and promote_symtype would not make sense in TermInterface.jl because other dependents do not use it.

@YingboMa the issue you've reported makes sense though and should be fixed.

Since maketerm will most likely be a dynamic dispatch anyways, does it make sense to specialize this on the operation (in this case typeof(==) to infer the correct symtype?

[0x0f0f0f]

Term manipulation could change symtype. If we don't want maketerm to take symtype, we should call promote_symtype but that's a dynamic dispatch and expensive. Should we make symtype a keyword argument that defaults to promote_symtype?

Mmmmh. This is specific to SymbolicUtils.jl and promote_symtype would not make sense in TermInterface.jl because other dependents do not use it.

@YingboMa the issue you've reported makes sense though and should be fixed.

Since maketerm will most likely be a dynamic dispatch anyways, does it make sense to specialize this on the operation (in this case typeof(==) to infer the correct symtype?

@YingboMa @ChrisRackauckas I'm preparing for conference tomorrow. I can't update at the moment, but happy to do in the next weeks/days if I find some time.

[ChrisRackauckas]

@bowenszhu is your #615 reliant on this?

[bowenszhu]

@bowenszhu is your #615 reliant on this?

No. It’s not.

[0x0f0f0f]

@ChrisRackauckas @YingboMa @bowenszhu ready for review. I think it'll pass CI

[bowenszhu]

In the commit e9ebd8f, you replaced all instances of arguments with sorted_arguments, which was not fully addressed in the subsequent merge commit bfb672d. The intention behind updating arguments was to avoid unnecessary sorting, as it is computationally expensive and not required in many cases.

[0x0f0f0f]

In the commit e9ebd8f, you replaced all instances of arguments with sorted_arguments, which was not fully addressed in the subsequent merge commit bfb672d. The intention behind updating arguments was to avoid unnecessary sorting, as it is computationally expensive and not required in many cases.

Yeah, I wanted to address them one-by-one, but I guess you did already in your MR? I can align to your changes then.

[ChrisRackauckas]

Yes that PR already went one by one to make the choices so just match that

[0x0f0f0f]

Yes that PR already went one by one to make the choices so just match that

I went to the PRs side by side and reverted the non-fundamental sorted_arguments. However I think that many of these cases will cause correctness issues in the futures, and may be already holding flakily because the possible permutations of small dictionaries (multisets with unordered dicts) are few. The fuzzing is supposed to cover this but I'm not sure how many functions the actual tests check

[0x0f0f0f]

Ready for review again

[0x0f0f0f]

@ChrisRackauckas @bowenszhu any news for this PR?

[0x0f0f0f]

In the commit e9ebd8f, you replaced all instances of arguments with sorted_arguments, which was not fully addressed in the subsequent merge commit bfb672d. The intention behind updating arguments was to avoid unnecessary sorting, as it is computationally expensive and not required in many cases.

@bowenszhu this was addressed

[0x0f0f0f]

Hey guys @ChrisRackauckas @bowenszhu ping

[bowenszhu]

This looks good! To ensure we're fully testing the new cases, could we add tests to reach 100% branch coverage here?

[0x0f0f0f]

Do you have any idea for a test structure? Also #565

[bowenszhu]

Since codecov is currently disabled, we probably have to do a thorough manual review of the code and tests to ensure all branches are adequately covered.