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

Remove method ambiguity in isequal(::Symbolic, ::Missing). #639

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Conversation

zengmao
Copy link
Contributor

@zengmao zengmao commented Aug 27, 2024

According to the linter Aqua.jl, SymbolicsUtils has method ambiguities associated with isequal and promote_symtype:

julia> Aqua.test_ambiguities(SymbolicUtils)
50 ambiguities found. To get a list, set `broken = false`.
Ambiguity #1
isequal(x, ::SymbolicUtils.Symbolic) @ SymbolicUtils ~/.julia/dev/SymbolicUtils/src/types.jl:227
isequal(::Missing, ::Any) @ Base missing.jl:81

Possible fix, define
  isequal(::Missing, ::SymbolicUtils.Symbolic)

Ambiguity #2
isequal(::SymbolicUtils.Symbolic, x) @ SymbolicUtils ~/.julia/dev/SymbolicUtils/src/types.jl:226
isequal(::Any, ::Missing) @ Base missing.jl:82

Possible fix, define
  isequal(::SymbolicUtils.Symbolic, ::Missing)

# 48 more ambiguities omitted, associated with `promte_symtype`.

This PR removes the ambiguities associated with isequal by defining the specific methods,

Base.isequal(::Missing, ::SymbolicUtils.BasicSymbolic{Real}) = false
Base.isequal(::SymbolicUtils.BasicSymbolic{Real}, ::Missing) = false

Copy link
Contributor

Benchmark Results

master 0288135... master/0288135946eb66...
overhead/acrule/a+2 0.725 ± 0.017 μs 0.756 ± 0.02 μs 0.96
overhead/acrule/a+2+b 0.713 ± 0.019 μs 0.726 ± 0.018 μs 0.982
overhead/acrule/a+b 0.248 ± 0.013 μs 0.258 ± 0.013 μs 0.961
overhead/acrule/noop:Int 25.6 ± 0.051 ns 25.9 ± 0.92 ns 0.988
overhead/acrule/noop:Sym 0.0367 ± 0.0057 μs 0.0365 ± 0.0053 μs 1.01
overhead/rule/noop:Int 0.0443 ± 0.00052 μs 0.0443 ± 0.00069 μs 1
overhead/rule/noop:Sym 0.0557 ± 0.0034 μs 0.0554 ± 0.0029 μs 1
overhead/rule/noop:Term 0.055 ± 0.0029 μs 0.0556 ± 0.0028 μs 0.989
overhead/ruleset/noop:Int 0.133 ± 0.0042 μs 0.129 ± 0.0029 μs 1.03
overhead/ruleset/noop:Sym 0.15 ± 0.0059 μs 0.148 ± 0.0047 μs 1.02
overhead/ruleset/noop:Term 3.12 ± 0.14 μs 3.1 ± 0.2 μs 1.01
overhead/simplify/noop:Int 0.137 ± 0.0012 μs 0.147 ± 0.0016 μs 0.933
overhead/simplify/noop:Sym 0.155 ± 0.0057 μs 0.174 ± 0.0059 μs 0.889
overhead/simplify/noop:Term 0.037 ± 0.0022 ms 0.038 ± 0.0026 ms 0.975
overhead/simplify/randterm (+, *):serial 0.09 ± 0.0018 s 0.0909 ± 0.0016 s 0.991
overhead/simplify/randterm (+, *):thread 0.052 ± 0.033 s 0.0533 ± 0.032 s 0.975
overhead/simplify/randterm (/, *):serial 0.216 ± 0.0072 ms 0.221 ± 0.0075 ms 0.977
overhead/simplify/randterm (/, *):thread 0.245 ± 0.0084 ms 0.251 ± 0.0088 ms 0.977
overhead/substitute/a 0.0591 ± 0.0017 ms 0.0618 ± 0.0017 ms 0.958
overhead/substitute/a,b 0.052 ± 0.0016 ms 0.0558 ± 0.0018 ms 0.932
overhead/substitute/a,b,c 17.1 ± 0.67 μs 18.7 ± 0.81 μs 0.915
polyform/easy_iszero 29.3 ± 2 μs 30 ± 1.9 μs 0.977
polyform/isone 2.79 ± 0.01 ns 2.79 ± 0 ns 0.996
polyform/iszero 1.14 ± 0.041 ms 1.16 ± 0.035 ms 0.983
polyform/simplify_fractions 1.66 ± 0.049 ms 1.71 ± 0.045 ms 0.967
time_to_load 2.15 ± 0.0074 s 2.16 ± 0.012 s 0.994

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

@ChrisRackauckas ChrisRackauckas merged commit a4e55b2 into JuliaSymbolics:master Aug 27, 2024
8 of 10 checks passed
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (65c0293) to head (0288135).
Report is 186 commits behind head on master.

Files Patch % Lines
src/types.jl 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   83.14%   81.86%   -1.29%     
==========================================
  Files          16       16              
  Lines        1869     1908      +39     
==========================================
+ Hits         1554     1562       +8     
- Misses        315      346      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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