Skip to content

Add tests for arithmetic on arrays#692

Open
ojwoodford wants to merge 6 commits intoJuliaSymbolics:masterfrom
ojwoodford:fix-array-arithmetic
Open

Add tests for arithmetic on arrays#692
ojwoodford wants to merge 6 commits intoJuliaSymbolics:masterfrom
ojwoodford:fix-array-arithmetic

Conversation

@ojwoodford
Copy link

Basic arithmetic involving arrays and scalars is broken (see #691). The goal of this PR is to fix it.

I'm new to Julia, and unfortunately understanding the internals of Symbolics.jl is beyond me. I've added some tests that catch the error. I'd love to collaborate with someone on fixes.

@ChrisRackauckas
Copy link
Member

Is this supposed to be listed as WIP, or just add to the tests?

@ojwoodford
Copy link
Author

Is this supposed to be listed as WIP, or just add to the tests?

WIP

@ojwoodford ojwoodford changed the title Fix #691 [WIP] Fix #691 Aug 9, 2022
@shashi
Copy link
Member

shashi commented Aug 12, 2022

Thanks these should act as test for #696

@shashi
Copy link
Member

shashi commented Aug 12, 2022

I think the left hand side of isequal needs Symbolics.scalarize calls.

julia> y[1] / y[2]
(broadcast(*, x, Ref(s)))[1] / (broadcast(*, x, Ref(s)))[2]

julia> Symbolics.scalarize(y[1] / y[2])
x[1] / x[2]

@ojwoodford ojwoodford changed the title [WIP] Fix #691 Add tests for arithmetic on arrays Aug 12, 2022
@ojwoodford
Copy link
Author

@shashi Done. Are you able to approve the workflows?

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #692 (c16881a) into master (0d7db80) will decrease coverage by 67.59%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #692       +/-   ##
==========================================
- Coverage   76.49%   8.90%   -67.60%     
==========================================
  Files          23      23               
  Lines        2719    2651       -68     
==========================================
- Hits         2080     236     -1844     
- Misses        639    2415     +1776     
Impacted Files Coverage Δ
src/groebner_basis.jl 0.00% <0.00%> (-100.00%) ⬇️
src/semipoly.jl 0.00% <0.00%> (-91.00%) ⬇️
src/linear_algebra.jl 0.00% <0.00%> (-89.48%) ⬇️
src/array-lib.jl 0.00% <0.00%> (-80.34%) ⬇️
src/diff.jl 0.37% <0.00%> (-80.00%) ⬇️
src/complex.jl 0.00% <0.00%> (-75.00%) ⬇️
src/build_function.jl 0.00% <0.00%> (-74.70%) ⬇️
src/difference.jl 0.00% <0.00%> (-70.00%) ⬇️
src/utils.jl 7.31% <0.00%> (-68.47%) ⬇️
src/arrays.jl 11.13% <0.00%> (-68.34%) ⬇️
... and 11 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ojwoodford
Copy link
Author

ojwoodford commented Aug 14, 2022

Shouldn't isequal on symbolic expressions resolve this kind of kind of problem?: isequal(s - x[2] + x[1] - s, x[1] - x[2]) returns false
In fact, if I write the expression above, it does return true. It's only when I write:

@variables x[1:2] s
y = x .+ s
@test isequal(scalarize(y[1] - y[2]), x[1] - x[2])

that the test throws an error. This is not the expected behaviour.

@ojwoodford
Copy link
Author

The problem has been suggested in #704

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.

4 participants