search

JuliaSymbolics / SymbolicUtils.jl

Symbolic expressions, rewriting and simplification
https://docs.sciml.ai/SymbolicUtils/stable/
Other
523 stars 99 forks source link

TermInterface Version 2 #609

Open 0x0f0f0f opened 3 weeks ago

0x0f0f0f commented 3 weeks ago

Have to adjust docs

github-actions[bot] commented 3 weeks ago

Benchmark Results

master 9f72fe641d1b86... master/9f72fe641d1b86...
overhead/acrule/a+2 0.729 ± 0.025 μs 0.742 ± 0.022 μs 0.983
overhead/acrule/a+2+b 0.697 ± 0.02 μs 0.72 ± 0.021 μs 0.968
overhead/acrule/a+b 0.25 ± 0.011 μs 0.269 ± 0.0099 μs 0.93
overhead/acrule/noop:Int 24.7 ± 0.059 ns 25.3 ± 0.051 ns 0.975
overhead/acrule/noop:Sym 0.0405 ± 0.0052 μs 0.0368 ± 0.0053 μs 1.1
overhead/rule/noop:Int 0.0443 ± 0.00074 μs 0.0447 ± 0.00055 μs 0.993
overhead/rule/noop:Sym 0.0584 ± 0.0035 μs 0.0553 ± 0.0026 μs 1.06
overhead/rule/noop:Term 0.0581 ± 0.0034 μs 0.0556 ± 0.0023 μs 1.05
overhead/ruleset/noop:Int 0.132 ± 0.0025 μs 0.13 ± 0.003 μs 1.02
overhead/ruleset/noop:Sym 0.153 ± 0.0068 μs 0.155 ± 0.0058 μs 0.986
overhead/ruleset/noop:Term 3.79 ± 0.15 μs 3.69 ± 0.16 μs 1.03
overhead/simplify/noop:Int 0.148 ± 0.0027 μs 0.145 ± 0.0021 μs 1.02
overhead/simplify/noop:Sym 0.149 ± 0.0036 μs 0.166 ± 0.0045 μs 0.898
overhead/simplify/noop:Term 0.0375 ± 0.0018 ms 0.0408 ± 0.0023 ms 0.92
overhead/simplify/randterm (+, *):serial 0.0924 ± 0.0012 s 0.0963 ± 0.0016 s 0.959
overhead/simplify/randterm (+, *):thread 0.0529 ± 0.037 s 0.0543 ± 0.03 s 0.975
overhead/simplify/randterm (/, *):serial 0.225 ± 0.0076 ms 0.237 ± 0.0082 ms 0.952
overhead/simplify/randterm (/, *):thread 0.257 ± 0.0093 ms 0.27 ± 0.009 ms 0.953
overhead/substitute/a 0.0573 ± 0.0017 ms 0.0664 ± 0.0019 ms 0.863
overhead/substitute/a,b 0.0511 ± 0.0018 ms 0.0579 ± 0.0016 ms 0.883
overhead/substitute/a,b,c 17.2 ± 0.79 μs 17.3 ± 0.8 μs 0.991
polyform/easy_iszero 0.0348 ± 0.0022 ms 0.0335 ± 0.0022 ms 1.04
polyform/isone 2.79 ± 0.011 ns 2.79 ± 0.01 ns 1
polyform/iszero 1.26 ± 0.045 ms 1.28 ± 0.038 ms 0.991
polyform/simplify_fractions 1.89 ± 0.061 ms 1.8 ± 0.048 ms 1.05
time_to_load 4.72 ± 0.13 s 4.56 ± 0.042 s 1.04

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

@bowenszhu is your https://github.com/JuliaSymbolics/SymbolicUtils.jl/pull/615 reliant on this?

bowenszhu commented 1 week ago

@bowenszhu is your #615 reliant on this?

No. It’s not.

0x0f0f0f commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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 commented 6 days ago

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

0x0f0f0f commented 6 days ago

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 commented 6 days ago

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

0x0f0f0f commented 6 days ago

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 commented 1 day ago

Ready for review again