search

JuliaSymbolics / SymbolicUtils.jl

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

New TermInterface #584

Closed 0x0f0f0f closed 5 months ago

shashi commented 6 months ago

The way I implemented children is by making it return the operation and arguments together. The head for BasicSymbolics was basicsymbolic. See: https://github.com/JuliaSymbolics/SymbolicUtils.jl/compare/s/terminterface1?expand=1

shashi commented 6 months ago

@0x0f0f0f does that make sense? I wanna finish this up and try out MT on SU expressions

0x0f0f0f commented 6 months ago

@0x0f0f0f does that make sense? I wanna finish this up and try out MT on SU expressions

This is up to you, if you think it makes more sense to have it implemented as such, please go ahead.

0x0f0f0f commented 6 months ago

I'll have a call with @nmheim shortly, would you like to join?

0x0f0f0f commented 6 months ago

@shashi adjusted with your proposal - CI passing now. documentation job is broken. disabled fuzzing tests as they segfault until https://github.com/JuliaMath/SpecialFunctions.jl/issues/446 is fixed

0x0f0f0f commented 6 months ago

Why is Symbolics.jl integration test failing even tho it has a compact for SymbolicUtils = "1.4" and the version is set to 1.6 here?

github-actions[bot] commented 5 months ago

Benchmark Results

master ed97a96a8864c5... master/ed97a96a8864c5...
overhead/acrule/a+2 0.724 ± 0.018 μs 0.715 ± 0.017 μs 1.01
overhead/acrule/a+2+b 0.723 ± 0.022 μs 0.71 ± 0.018 μs 1.02
overhead/acrule/a+b 0.271 ± 0.01 μs 0.257 ± 0.0097 μs 1.05
overhead/acrule/noop:Int 25 ± 0.05 ns 25.9 ± 0.05 ns 0.965
overhead/acrule/noop:Sym 0.0339 ± 0.004 μs 0.0339 ± 0.0044 μs 1
overhead/rule/noop:Int 0.0378 ± 0.00042 μs 0.0377 ± 0.001 μs 1
overhead/rule/noop:Sym 0.0421 ± 0.0016 μs 0.042 ± 0.0013 μs 1
overhead/rule/noop:Term 0.0427 ± 0.0016 μs 0.0426 ± 0.0014 μs 1
overhead/ruleset/noop:Int 0.123 ± 0.002 μs 0.121 ± 0.0021 μs 1.02
overhead/ruleset/noop:Sym 0.14 ± 0.0038 μs 0.142 ± 0.0032 μs 0.988
overhead/ruleset/noop:Term 3.38 ± 0.17 μs 6.54 ± 0.41 μs 0.517
overhead/simplify/noop:Int 0.154 ± 0.002 μs 0.145 ± 0.0036 μs 1.06
overhead/simplify/noop:Sym 0.171 ± 0.003 μs 0.153 ± 0.0027 μs 1.12
overhead/simplify/noop:Term 0.0392 ± 0.0022 ms 0.0448 ± 0.0025 ms 0.874
overhead/simplify/randterm (+, *):serial 0.118 ± 0.002 s 0.135 ± 0.0064 s 0.878
overhead/simplify/randterm (+, *):thread 0.0739 ± 0.025 s 0.0837 ± 0.024 s 0.883
overhead/simplify/randterm (/, *):serial 0.224 ± 0.0077 ms 0.272 ± 0.01 ms 0.823
overhead/simplify/randterm (/, *):thread 0.256 ± 0.0089 ms 0.317 ± 0.011 ms 0.806
overhead/substitute/a 0.06 ± 0.0015 ms 0.12 ± 0.0032 ms 0.5
overhead/substitute/a,b 0.0523 ± 0.0014 ms 0.101 ± 0.0029 ms 0.518
overhead/substitute/a,b,c 16.4 ± 0.7 μs 16.9 ± 0.68 μs 0.972
polyform/easy_iszero 0.0317 ± 0.0017 ms 0.0551 ± 0.0024 ms 0.575
polyform/isone 2.79 ± 0.01 ns 2.79 ± 0.01 ns 1
polyform/iszero 1.77 ± 0.039 ms 5.53 ± 0.1 ms 0.32
polyform/simplify_fractions 2.36 ± 0.047 ms 3.38 ± 0.057 ms 0.697
time_to_load 4.58 ± 0.028 s 4.04 ± 0.016 s 1.13

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 5 months ago

Downstream MTK Failing due to deprecation warnings

 Discrete System: Test Failed at /cache/build/builder-amdci5-1/julialang/julia-release-1-dot-10/usr/share/julia/stdlib/v1.10/Test/src/Test.jl:903
  Expression: isempty(stderr_content)
   Evaluated: isempty("WARNING: ModelingToolkit.istree is deprecated, use TermInterface.iscall instead.\n  likely near /home/runner/work/SymbolicUtils.jl/SymbolicUtils.jl/downstream/test/discrete_system.jl:267\nin collect_var_to_name! at /home/runner/work/SymbolicUtils.jl/SymbolicUtils.jl/downstream/src/utils.jl\n")

I suggested to remove deprecation warnings and do a major release bump

0x0f0f0f commented 5 months ago

I suggested to remove deprecation warnings and do a major release bump

Ok, there was already a major version bump, shouldn't downstream CI not be executed?

0x0f0f0f commented 5 months ago

@shashi any news on the state of this?

shashi commented 5 months ago

I think we can release it as a major release and update downstream. The errors are all because of the depwarn. Some packages are testing the if stderr is empty.

shashi commented 5 months ago

I'll make the relevant PRs to Symbolics and MTK today.

devmotion commented 4 months ago

Shouldn't this have been a breaking major release? It completely broke my project (that due to Catalyst still has to use MTK v8) when running Pkg.update().

0x0f0f0f commented 4 months ago

Shouldn't this have been a breaking major release? It completely broke my project (that due to Catalyst still has to use MTK v8) when running Pkg.update().

@shashi I think this should have been a major breaking release. It has also been reported by a colleague of mine. Lots of deprecation warnings. I think we should just do a major release and drop similarterm alltogether and use maketerm