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.

Sign up for GitHub

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

Jump to bottom

setu does not perform any sizecheck #82

Open
hexaeder opened this issue May 31, 2024 · 1 comment · May be fixed by #83
Open

setu does not perform any sizecheck #82

hexaeder opened this issue May 31, 2024 · 1 comment · May be fixed by #83
Labels
bug Something isn't working

Comments

@hexaeder
Copy link

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 hexaeder added the bug Something isn't working label May 31, 2024
@hexaeder
Copy link
Author

hexaeder commented Jun 3, 2024

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

SymbolicIndexingInterface.jl/src/parameter_indexing.jl

Lines 206 to 208 in c9c7b6c

function (ms::MultipleSetters)(prob, val)
map((s!, v) -> s!(prob, v), ms.setters, val)
end

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

hexaeder added a commit to hexaeder/SymbolicIndexingInterface.jl that referenced this issue Jun 3, 2024
@hexaeder
broadcast MultipleSetters instead of map
c7a972f 
fixes SciML#82
hexaeder added a commit to hexaeder/SymbolicIndexingInterface.jl that referenced this issue Jun 3, 2024
@hexaeder
broadcast MultipleSetters instead of map
426cdf9 
fixes SciML#82
@hexaeder hexaeder linked a pull request Jun 3, 2024 that will close this issue
Open
5 tasks
hexaeder added a commit to hexaeder/SymbolicIndexingInterface.jl that referenced this issue Jun 14, 2024
@hexaeder
broadcast MultipleSetters instead of map
cefe510 
fixes SciML#82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

check dimensions for multiple setters hexaeder/SymbolicIndexingInterface.jl
1 participant
@hexaeder