TermInterface Version 2

[0x0f0f0f]

[0x0f0f0f]

Have to adjust docs

[github-actions[bot]]

Benchmark Results

	master	b11fbec6bfc3a9...	master/b11fbec6bfc3a9...
overhead/acrule/a+2	0.738 ± 0.019 μs	0.742 ± 0.019 μs	0.995
overhead/acrule/a+2+b	0.713 ± 0.021 μs	0.729 ± 0.022 μs	0.978
overhead/acrule/a+b	0.27 ± 0.012 μs	0.263 ± 0.012 μs	1.02
overhead/acrule/noop:Int	25.6 ± 0.05 ns	25.1 ± 0.92 ns	1.02
overhead/acrule/noop:Sym	0.0368 ± 0.0054 μs	0.0366 ± 0.0054 μs	1
overhead/rule/noop:Int	0.0454 ± 0.0011 μs	0.0449 ± 0.001 μs	1.01
overhead/rule/noop:Sym	0.0544 ± 0.0028 μs	0.0563 ± 0.003 μs	0.966
overhead/rule/noop:Term	0.0552 ± 0.0033 μs	0.0559 ± 0.0028 μs	0.987
overhead/ruleset/noop:Int	0.131 ± 0.0035 μs	0.128 ± 0.003 μs	1.02
overhead/ruleset/noop:Sym	0.152 ± 0.0042 μs	0.159 ± 0.0048 μs	0.955
overhead/ruleset/noop:Term	3.46 ± 0.15 μs	3.28 ± 0.15 μs	1.05
overhead/simplify/noop:Int	0.153 ± 0.0045 μs	0.143 ± 0.0023 μs	1.07
overhead/simplify/noop:Sym	0.165 ± 0.0067 μs	0.157 ± 0.0055 μs	1.06
overhead/simplify/noop:Term	0.038 ± 0.0018 ms	0.0369 ± 0.0019 ms	1.03
overhead/simplify/randterm (+, *):serial	0.0906 ± 0.0012 s	0.0871 ± 0.0016 s	1.04
overhead/simplify/randterm (+, *):thread	0.052 ± 0.03 s	0.0515 ± 0.031 s	1.01
overhead/simplify/randterm (/, *):serial	0.218 ± 0.0062 ms	0.21 ± 0.0078 ms	1.04
overhead/simplify/randterm (/, *):thread	0.252 ± 0.0081 ms	0.236 ± 0.0086 ms	1.07
overhead/substitute/a	0.0541 ± 0.0015 ms	0.0599 ± 0.0018 ms	0.903
overhead/substitute/a,b	0.0486 ± 0.0014 ms	0.0531 ± 0.0021 ms	0.916
overhead/substitute/a,b,c	17.4 ± 0.71 μs	16.7 ± 0.71 μs	1.04
polyform/easy_iszero	31.3 ± 1.7 μs	28.4 ± 2.2 μs	1.1
polyform/isone	2.79 ± 0.01 ns	2.79 ± 0.01 ns	1
polyform/iszero	1.17 ± 0.031 ms	1.12 ± 0.033 ms	1.05
polyform/simplify_fractions	1.8 ± 0.041 ms	1.6 ± 0.04 ms	1.12
time_to_load	4.52 ± 0.029 s	4.64 ± 0.058 s	0.975

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

[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 https://github.com/JuliaSymbolics/SymbolicUtils.jl/pull/615 reliant on this?

[bowenszhu]

@bowenszhu is your #615 reliant on this?

No. It’s not.

[0x0f0f0f]

@bowenszhu can we please also add the changes for sorted/unsorted arguments from #615 here? I didn't change many callsites of arguments, just low-hanging fruit like node_count. This is blocking my work.

[0x0f0f0f]

@ChrisRackauckas will break downstream in the same way #615 does. I think it really makes sense to update downstream a single time, otherwise we will need to have 6 PRs (3 for terminterface and 3 for #615), one each for SU, Symbolics and MTK. Please let's synchronise work somehow

[ChrisRackauckas]

My understanding is that PR is aiming to not be breaking downstream. Can you rebase on top of it? The deprecation paths and such would be very good to have.

[YingboMa]

The implementation detail of make_term has nothing to do with TermInterface. Just call promote_symtype there to be correct, otherwise, this PR is not acceptable.

[0x0f0f0f]

The implementation detail of make_term has nothing to do with TermInterface. Just call promote_symtype there to be correct, otherwise, this PR is not acceptable.

@YingboMa will do. Wanted to catch up with #615 first.

[0x0f0f0f]

My understanding is that PR is aiming to not be breaking downstream. Can you rebase on top of it? The deprecation paths and such would be very good to have.

@ChrisRackauckas sure. Will add correct deprecation paths as next step after fixing maketerm. At PLDI24 now.

[0x0f0f0f]

@YingboMa I ended up having to do this. I don't really like it, but it would be OK temporarily. Otherwise tests were failing as it was expecting Number in some tests and stuff got promoted to Real instead.

function TermInterface.maketerm(T::Type{<:BasicSymbolic}, head, args, metadata)
    st = symtype(T)
    pst = _promote_symtype(head, args)
    # Use promoted symtype only if not a subtype of the existing symtype of T.
    # This is useful when calling `maketerm(BasicSymbolic{Number}, (==), [true, false])` 
    # Where the result would have a symtype of Bool. 
    # Please see discussion in https://github.com/JuliaSymbolics/SymbolicUtils.jl/pull/609 
    # TODO this should be optimized.
    new_st = if pst === Bool 
        pst 
    elseif pst === Any || (st === Number && pst <: st) 
        st
    else 
        pst 
    end 
    basicsymbolic(head, args, new_st, metadata)
end

Do you have anything better in mind?

[0x0f0f0f]

Thanks for working on this! I have a couple of suggestions that might make reviewing this PR easier:

1. **Scope:** Could we focus this PR on migrating to TermInterface v1 first? Migrating directly to v2 could be tackled in a separate PR. This would help with reviewing and isolating any potential issues.

2. **Commit [e9ebd8f](https://github.com/JuliaSymbolics/SymbolicUtils.jl/commit/e9ebd8f56fcc0ddfc7c2510cefd55581f125df9b):** I noticed this commit copies code from PR [Optimize `arguments` function by removing sorting #615](https://github.com/JuliaSymbolics/SymbolicUtils.jl/pull/615) instead of merging it. Would merging be a better approach here to keep the history cleaner and avoid merge conflicts?

If you're set on migrating directly to v2 and need a hand resolving merge conflicts, let me know! I'm happy to help.

Hey thanks for the tips. For scope point 1) I guess that it would be incompatible for the way you defined arguments with a sort kwarg. I'll merge everything from master branch

[0x0f0f0f]

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

[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

[0x0f0f0f]

@bowenszhu added tests covering new maketerm symtype propagation in latest commit