Open Sacha0 opened 7 years ago
Bifurcation sounds good, guess I'd prefer fill
and fillnz
.
There is a fillslots!
helper function but it's not exported yet I believe. I think fillstored!
would be a somewhat better name.
fillnz!
seems ambiguous: What should fillnz!(Diagonal([0, 0, 0]), 1)
do? Yield Diagonal([0, 0, 0])
or Diagonal([1, 1, 1])
? Best!
I would reverse the convention: fill! = fillstored!
a separate fillall!
. I think that leads to easier dense-sparse generalizations (at least for the class of problems I usually deal with). Densifying a sparse matrix should be opt-in.
fill!(A, 1)
not meaning the same thing as A[:] = 1
would be really problematic and error-prone from a generic programming perspective. Getting storage-dependent behavior is the non-generic case and should not be the standard API.
I would reverse the convention: fill! = fillstored! a separate fillall!. I think that leads to easier dense-sparse generalizations (at least for the class of problems I usually deal with). Densifying a sparse matrix should be opt-in.
On the one hand I like this argument. Being explicit (fillstored!
/fillall!
) and using fillstored!
for existing uses of fill!
strikes me as worth typing the extra few characters though.
On the other hand I also like @tkelman
's argument. Which makes the explicit names seem still more attractive, requiring the author to make clear her intention.
A think a good way to address this is to use representations of the relevant indices. For example, instead of having Xstored!
for many functions X
, do something like A[stored(A)] = 1
. There has recently been some related discussion here: https://github.com/JuliaComputing/NDSparseData.jl/issues/23
Also in https://github.com/timholy/ArrayIteration.jl @timholy has been experimenting with these kinds of abstractions.
For one-liners like fill!
the stored(A)
approach looks good, but for more involved operations the question of how to define the Xstored
, Xall
, and X
methods is still relevant.
@tkelman yeah, breaking that identity would be bad, although I'm not sure using colons on sparse matrices is a good idea at all. I guess the next best thing would be fill! = fillall!
for sparse matrices and make fillstored!
fall back to fill!
for dense matrices.
As a general principle, I think the sparse-specific functions should have fallbacks for the dense case to enable generic programming.
I'm not sure using colons on sparse matrices is a good idea at all.
It isn't, but unless it's going to be a method error I think we should keep the existing "linear algebraic" concepts consistent between dense and sparse, as most of the AbstractArray interface tends to be designed around that. New abstractions for storage-dependent behavior should probably be the special cases, and need careful thought to migrate code to use them correctly - somewhat like (otherwise dense) unconventionally-indexed arrays.
Per triage, the long-term direction should be fill!(stored(A), val)
. For the short term, perhaps fillslots!
-> fillstored!
and remains unexported from LinAlg
. Best!
+1 fillstored!
and eventually fill!(stored(A), val)
.
Bumping on this. I just hit it, and from the looks of it it seems other scientific computing groups are hitting it too. It's very non-intuitive for fill!
to densify.
Ref. discussion between @martinholters and @tkelman starting at https://github.com/JuliaLang/julia/pull/16740#issuecomment-224578978. How should
fill!
behave for storage types with structural constraints such asBidiagonal
andSparseMatrixCSC
? Brief illustration:In other words, does
fill!
mean 'fill every addressable entry in the providedAbstractArray
', or 'fill every stored entry in the providedAbstractArray
'? Shouldfill!
bifurcate into, e.g.,fillall!
andfillstored!
? Best!