Skip to content

Add setindex! #51

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add setindex! #51

wants to merge 2 commits into from

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Apr 16, 2025

First commit has a version of the setindex! method suggested at #6 (comment), which works like this:

julia> x = onehotbatch(1:3, 0:4)
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
 1    
   1  
     1
     

julia> x[1] = 1;  # really this changes two values, OK?

julia> x[1,3] = 0;  # no effect

julia> x
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
 1    
     
   1  
     1
     

julia> copyto!(x, onehotbatch([4,3,2], 0:4));  # calls setindex! 15 times

julia> x
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
     
     1
   1  
 1    

julia> x .= 0  # this is bad, illegal state?
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
     
     
     
     

Allowing you to replace the only 1 by 0 is what makes the copyto! example work, where this is a temporary state. But it also allows x .= 0 as in the last example, which leaves the array in a state we otherwise disallow:

julia> onehotbatch(1:10, 0:4, 0)  # with default
5×10 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
         1  1  1  1  1  1
 1                  
   1                
     1              
       1            

julia> onehotbatch(1:10, 0:4)  # without default
ERROR: Value 10 not found in labels

I forgot about #10, which instead makes writing 0 always an error.

This comment #6 (comment) suggested only allowing things like the following -- where a single setindex! call can be sure never to leave the array in an illegal state:

julia> x = onehotbatch(1:3, 0:4);

julia> x[:,3] = collect(onehot(0, 0:4))  # setindex! with array eventually calls this PR's scalar setindex! 5 times
5-element Vector{Bool}:
 1
 0
 0
 0
 0

julia> x
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     1
 1    
   1  
     
     

PR Checklist

  • Tests are added
  • Documentation, if applicable

@mcabbott
Copy link
Member Author

Second commit makes overwriting a 1 with a 0 increment the label. Then copyto! still works, and x .= 0 gives an error rather than illegal answer. Although x .= 1 is a little strange:

julia> x = onehotbatch(1:3, 0:4)
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
 1    
   1  
     1
     

julia> x[2] = 0  # changes 2 values, now
0

julia> x
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
     
 1  1  
     1
     

julia> x .= 1
5×3 OneHotMatrix(::Vector{UInt32}) with eltype Bool:
     
     
     
     
 1  1  1

julia> x .= 0
ERROR: ArgumentError: `setindex!` here would leave the `OneHotArray` without a hot one (in this column)

This comment was marked as off-topic.

@juliohm
Copy link

juliohm commented Apr 17, 2025

@mcabbott thank you for the PR. What is pending to get it merged?

@mcabbott
Copy link
Member Author

mcabbott commented Apr 17, 2025

Mostly we need to be convinced this is doing the right thing. There were many other behaviours proposed earlier.

Are we OK with x .= 1 not giving an error?

Are there other surprising cases I haven't thought of? Things which are now errors & will become silently wrong answers? This seems a bit like the question of whether setindex!(::Symmetric, ...) should work off-diagonal. But here, it's harder to imagine some generic function writing into a OneHotArray in a way which gives wrong answers. Maybe prod! is an example?

julia> mul!(rand(Bool,3,3), rand(Bool,3,3), rand(Bool,3,3))  # problem for Symmetric
ERROR: InexactError: Bool(2)

julia> x = rand(Bool,4,2)
4×2 Matrix{Bool}:
 0  1
 1  1
 1  0
 1  1

julia> prod!(rand(Bool,4), x)
4-element Vector{Bool}:
 0
 1
 0
 1

julia> prod!(onehotbatch(3, 1:4), x)  # with this PR, often an error, here silenty wrong
4-element OneHotVector(::Array{UInt32, 0}) with eltype Bool:
 
 
 
 1

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.

2 participants