SciML / SymbolicIndexingInterface.jl

A general interface for symbolic indexing of SciML objects used in conjunction with Domain-Specific Languages
https://docs.sciml.ai/SymbolicIndexingInterface/stable/
MIT License
15 stars 8 forks source link

`setu` does not perform any sizecheck #82

Open hexaeder opened 5 months ago

hexaeder commented 5 months ago

Describe the bug 🐞

setu does not perform sizecheck between the new values and the number of indexed elements.

Expected behavior

I would expect it to either error or implicitly broadcast, but it only overwrites a single value. The expectation comes especially when using the setindex! interface, because then it feels like array indexing

a = zeros(2)
a[[1,2]] = 1 # errors
a[[1,2]] .= 1 # overwrites both elements
a[[1,2]] = [1,2] # overwrites both elements

Minimal Reproducible Example 👇

inpr = SymbolCache([:x, :y])
ps = ProblemState(;u=zeros(2))

getu(inpr, [:x,:y])(ps) == zeros(2) # as expected
setu(inpr, [:x,:y])(ps, 1) # does not error, only overwrites first element
getu(inpr, [:x,:y])(ps) == [1,0] # strange?
hexaeder commented 4 months ago

I think this could be solved by replacing map with broadcast here:

https://github.com/SciML/SymbolicIndexingInterface.jl/blob/c9c7b6c1fd6fd3ac787e6a9d554d84efcd80f2e4/src/parameter_indexing.jl#L206-L208

Underlying issue being that mapping over multiple collections only loops through the shortest list. I think broadcast behaviour is what you'd expect here

julia> map((a,b)->a*repr(b), ["foo", "bar"], 1)
1-element Vector{String}:
 "foo1"

julia> map((a,b)->a*repr(b), ["foo", "bar"], [1,2])
2-element Vector{String}:
 "foo1"
 "bar2"

julia> map((a,b)->a*repr(b), ["foo", "bar"], [1,2,3])
2-element Vector{String}:
 "foo1"
 "bar2"

julia> broadcast((a,b)->a*repr(b), ["foo", "bar"], 1)
2-element Vector{String}:
 "foo1"
 "bar1"

julia> broadcast((a,b)->a*repr(b), ["foo", "bar"], [1,2])
2-element Vector{String}:
 "foo1"
 "bar2"

julia> broadcast((a,b)->a*repr(b), ["foo", "bar"], [1,2,3])
ERROR: DimensionMismatch: arrays could not be broadcast to a common size: a has axes Base.OneTo(2) and b has axes