Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing metadata when operation and resulting BasicSymbolic subtype mismatch #632

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

bowenszhu
Copy link
Member

@bowenszhu bowenszhu commented Aug 12, 2024

This PR addresses an issue in the basicsymbolic function where it incorrectly assumes that the + and * operations always result in Add and Mul objects, respectively.

if f === (+)
res = +(args...)
if isadd(res)
@set! res.metadata = metadata
end
res
elseif f == (*)
res = *(args...)
if ismul(res)
@set! res.metadata = metadata
end
res

Currently, when arguments possess metadata, the operations might return a Term object instead, as seen in

SymbolicUtils.jl/src/types.jl

Lines 1082 to 1083 in 2ec473e

function +(a::SN, b::SN)
!issafecanon(+, a,b) && return term(+, a, b) # Don't flatten if args have metadata

SymbolicUtils.jl/src/types.jl

Lines 1136 to 1138 in 2ec473e

function *(a::SN, b::SN)
# Always make sure Div wraps Mul
!issafecanon(*, a, b) && return term(*, a, b)

This inconsistency was causing problems in Vaibhavdixit02/SymbolicAnalysis.jl#43 (comment)

This PR corrects this problem by explicitly handling the Term case. This ensures the maketerm function behaves consistently and correctly handles arguments with metadata.

Copy link
Contributor

github-actions bot commented Aug 12, 2024

Benchmark Results

master d8327c0... master/d8327c054f981d...
overhead/acrule/a+2 0.833 ± 0.021 μs 0.733 ± 0.02 μs 1.14
overhead/acrule/a+2+b 0.816 ± 0.022 μs 0.722 ± 0.016 μs 1.13
overhead/acrule/a+b 0.264 ± 0.0083 μs 0.267 ± 0.011 μs 0.989
overhead/acrule/noop:Int 25.9 ± 0.91 ns 26.2 ± 0.92 ns 0.988
overhead/acrule/noop:Sym 0.0369 ± 0.0055 μs 0.0367 ± 0.0061 μs 1
overhead/rule/noop:Int 0.0444 ± 0.00051 μs 0.0443 ± 0.0011 μs 1
overhead/rule/noop:Sym 0.0559 ± 0.0026 μs 0.0546 ± 0.0026 μs 1.02
overhead/rule/noop:Term 0.0558 ± 0.0025 μs 0.055 ± 0.0027 μs 1.02
overhead/ruleset/noop:Int 0.129 ± 0.0026 μs 0.129 ± 0.004 μs 0.999
overhead/ruleset/noop:Sym 0.157 ± 0.0055 μs 0.152 ± 0.0042 μs 1.03
overhead/ruleset/noop:Term 3.27 ± 0.16 μs 3.03 ± 0.11 μs 1.08
overhead/simplify/noop:Int 0.138 ± 0.0023 μs 0.14 ± 0.0028 μs 0.984
overhead/simplify/noop:Sym 0.153 ± 0.0066 μs 0.153 ± 0.006 μs 0.997
overhead/simplify/noop:Term 0.0369 ± 0.0023 ms 0.0359 ± 0.0019 ms 1.03
overhead/simplify/randterm (+, *):serial 0.0904 ± 0.0012 s 0.087 ± 0.0012 s 1.04
overhead/simplify/randterm (+, *):thread 0.0522 ± 0.028 s 0.0502 ± 0.031 s 1.04
overhead/simplify/randterm (/, *):serial 0.218 ± 0.0078 ms 0.212 ± 0.0072 ms 1.03
overhead/simplify/randterm (/, *):thread 0.247 ± 0.009 ms 0.242 ± 0.0081 ms 1.02
overhead/substitute/a 0.0636 ± 0.0016 ms 0.0583 ± 0.0016 ms 1.09
overhead/substitute/a,b 0.0559 ± 0.0015 ms 0.0503 ± 0.0014 ms 1.11
overhead/substitute/a,b,c 18.7 ± 0.75 μs 16.9 ± 0.68 μs 1.1
polyform/easy_iszero 30.2 ± 2 μs 28.5 ± 2 μs 1.06
polyform/isone 2.79 ± 0.01 ns 3.1 ± 0.01 ns 0.903
polyform/iszero 1.17 ± 0.032 ms 1.11 ± 0.031 ms 1.05
polyform/simplify_fractions 1.67 ± 0.045 ms 1.59 ± 0.039 ms 1.05
time_to_load 4.48 ± 0.021 s 4.46 ± 0.04 s 1.01

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

@bowenszhu bowenszhu force-pushed the fix-basicsymbolic-drop-metadata branch 2 times, most recently from 6d89149 to cfbfc4f Compare August 12, 2024 02:29
@bowenszhu bowenszhu force-pushed the fix-basicsymbolic-drop-metadata branch from cfbfc4f to d8327c0 Compare August 12, 2024 02:46
@Vaibhavdixit02
Copy link
Collaborator

Thank you @bowenszhu!!

@bowenszhu bowenszhu marked this pull request as ready for review August 12, 2024 03:29
Copy link
Collaborator

@Vaibhavdixit02 Vaibhavdixit02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

Copy link
Member

@YingboMa YingboMa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the correct fix. Thanks!

@YingboMa
Copy link
Member

@bowenszhu could you merge and release this fix when you feel comfortable?

@bowenszhu bowenszhu merged commit a81f234 into master Aug 12, 2024
10 of 12 checks passed
@bowenszhu bowenszhu deleted the fix-basicsymbolic-drop-metadata branch August 12, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants