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 maketerm handling of BasicSymbolic{Array} #631

Merged
merged 2 commits into from
Aug 11, 2024

Conversation

bowenszhu
Copy link
Member

@bowenszhu bowenszhu commented Aug 11, 2024

This PR addresses the test failure observed in https://github.com/JuliaSymbolics/Symbolics.jl/actions/runs/10335343905/job/28609768547?pr=1196#step:7:582 within PR JuliaSymbolics/Symbolics.jl#1196.

Currently, maketerm doesn't correctly handle cases where the input T is a BasicSymbolic{Array}. This leads to the resulting type being incorrectly changed to AbstractArray due to https://github.com/JuliaSymbolics/Symbolics.jl/blob/933757bfd1d04f2097c8ad84140796fb19dc3d91/src/arrays.jl#L437.

This PR modifies maketerm to properly handle BasicSymbolic{Array} inputs, ensuring the correct type is maintained.

However, we should probably refactor maketerm for symbolic arrays with array_term in Symbolics.jl later.

@bowenszhu bowenszhu changed the title Remove symtype function call in _promote_symtype to fix symbolic array type inference in Symbolics.jl Remove symtype function call in _promote_symtype to fix symbolic array container type inference in Symbolics.jl Aug 11, 2024
@bowenszhu bowenszhu closed this Aug 11, 2024
@bowenszhu bowenszhu reopened this Aug 11, 2024
@bowenszhu bowenszhu changed the title Remove symtype function call in _promote_symtype to fix symbolic array container type inference in Symbolics.jl Fix symbolic array container type inference for Symbolics.jl Aug 11, 2024
@bowenszhu bowenszhu force-pushed the fix-array-function-type-inference branch from cca377d to f0bd7e4 Compare August 11, 2024 01:14
Copy link
Contributor

github-actions bot commented Aug 11, 2024

Benchmark Results

master 8b52a80... master/8b52a80f06c65d...
overhead/acrule/a+2 0.734 ± 0.016 μs 0.732 ± 0.015 μs 1
overhead/acrule/a+2+b 0.711 ± 0.013 μs 0.708 ± 0.015 μs 1
overhead/acrule/a+b 0.261 ± 0.0074 μs 0.264 ± 0.0089 μs 0.991
overhead/acrule/noop:Int 25.3 ± 0.05 ns 26.2 ± 0.05 ns 0.965
overhead/acrule/noop:Sym 0.0367 ± 0.0057 μs 0.038 ± 0.0059 μs 0.967
overhead/rule/noop:Int 0.0439 ± 0.00061 μs 0.0446 ± 0.0012 μs 0.984
overhead/rule/noop:Sym 0.0546 ± 0.003 μs 0.0562 ± 0.0018 μs 0.971
overhead/rule/noop:Term 0.0541 ± 0.0027 μs 0.0561 ± 0.0017 μs 0.965
overhead/ruleset/noop:Int 0.128 ± 0.0035 μs 0.13 ± 0.0045 μs 0.983
overhead/ruleset/noop:Sym 0.157 ± 0.0039 μs 0.147 ± 0.0043 μs 1.06
overhead/ruleset/noop:Term 3.17 ± 0.11 μs 3.07 ± 0.14 μs 1.03
overhead/simplify/noop:Int 0.138 ± 0.0025 μs 0.161 ± 0.0016 μs 0.859
overhead/simplify/noop:Sym 0.157 ± 0.0023 μs 0.18 ± 0.0099 μs 0.873
overhead/simplify/noop:Term 0.0365 ± 0.0017 ms 0.0365 ± 0.0019 ms 1
overhead/simplify/randterm (+, *):serial 0.0871 ± 0.0018 s 0.0873 ± 0.001 s 0.998
overhead/simplify/randterm (+, *):thread 0.0511 ± 0.031 s 0.0509 ± 0.03 s 1
overhead/simplify/randterm (/, *):serial 0.213 ± 0.0053 ms 0.214 ± 0.0062 ms 0.996
overhead/simplify/randterm (/, *):thread 0.244 ± 0.0073 ms 0.242 ± 0.0078 ms 1.01
overhead/substitute/a 0.0599 ± 0.0014 ms 0.0585 ± 0.0015 ms 1.02
overhead/substitute/a,b 0.0537 ± 0.0015 ms 0.0522 ± 0.0014 ms 1.03
overhead/substitute/a,b,c 17.1 ± 0.71 μs 17.5 ± 0.74 μs 0.975
polyform/easy_iszero 29.2 ± 1.6 μs 29.3 ± 2 μs 0.999
polyform/isone 2.79 ± 0.01 ns 3.1 ± 0.01 ns 0.9
polyform/iszero 1.11 ± 0.028 ms 1.11 ± 0.03 ms 1.01
polyform/simplify_fractions 1.62 ± 0.037 ms 1.61 ± 0.038 ms 1.01
time_to_load 4.45 ± 0.01 s 4.44 ± 0.013 s 1

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 marked this pull request as ready for review August 11, 2024 01:39
@bowenszhu bowenszhu force-pushed the fix-array-function-type-inference branch from 9733094 to 8b52a80 Compare August 11, 2024 01:43
@bowenszhu bowenszhu changed the title Fix symbolic array container type inference for Symbolics.jl Fix maketerm handling of BasicSymbolic{Array} Aug 11, 2024
@bowenszhu bowenszhu merged commit d08d164 into master Aug 11, 2024
11 of 12 checks passed
@bowenszhu bowenszhu deleted the fix-array-function-type-inference branch August 11, 2024 01:59
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.

1 participant