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

ismutable wrong for FillArrays #77

Open
oxinabox opened this issue Oct 14, 2020 · 6 comments
Open

ismutable wrong for FillArrays #77

oxinabox opened this issue Oct 14, 2020 · 6 comments

Comments

@oxinabox
Copy link
Member

oxinabox commented Oct 14, 2020

FillArrays are not mutable.

julia> ArrayInterface.ismutable(typeof(Fill(4, (2,2))))
true

I think that
https://github.com/SciML/ArrayInterface.jl/blob/730592e19c5e3effb086a5cc0b61ecc1a1936a76/src/ArrayInterface.jl#L75-L81

Should be

 function ismutable(::Type{T}) where {T<:AbstractArray} 
     if parent_type(T) <: T 
         return false  # no parent, and have not hit a type we know about 
     else 
         return ismutable(parent_type(T)) 
     end 
 end

ismutable{::Type{<:Array}} = true
ismutable{::Type{<:SparseVector}} = true
ismutable{::Type{<:SparseMatrixCSC}} = true
ismutable{::Type{<:BitArray}} = true

Alternatively
the fallback case could be: return T.mutable rather than false.
On the assumption that if you are a mutable struct that has subtypes AbstractArray you probably have defined setindex

@Tokazama
Copy link
Member

I like the idea of mutability being something that needs to be opted into. I didn't originally make true the default, so I'm assuming that decision reflected that the majority of the ecosystem uses Array and parent_type wasn't a method at the time to dig into wrappers.

Does this break anything.

@chriselrod
Copy link
Collaborator

chriselrod commented Oct 14, 2020

Does this break anything.

How many libraries are currently using ArrayInterface.jl?
It's easy enough to test a small handful locally, like DifferentialEquations.jl.

Barring breaking a bunch of stuff, I think it's okay to make false the default.

@oxinabox
Copy link
Member Author

oxinabox commented Oct 14, 2020

basically anything that is mutable will eventually parent it's way up a fundermentally mutable backing array type, probably Array.
Immutable things, like StaticArrays and FillArrays tend not to have parents in the first place

@Tokazama
Copy link
Member

Tokazama commented Mar 7, 2022

Should we suggest using ArrayInterface.can_setindex and ArrayInterface.can_change_size in documentation for most arrays since these methods are more specific?

@ChrisRackauckas
Copy link
Member

No, can_setindex is different, for example, with GPUs.

@ChrisRackauckas
Copy link
Member

I assume all AbstractFill are immutable? If that's true, we should just make an extension library for FillArrays that does that. I'll wait until someone who knows FillArrays.jl better responds and makes sure that's true.

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

No branches or pull requests

4 participants