Add NEWS and docs to encourage accessor usage for sparse arrays #33050
Conversation
| @@ -50,6 +54,12 @@ function nonzeros(x::SparseColumnView) | |||
| return y | |||
| end | |||
|
|
|||
| """ | |||
| SparseArrays.nonzeroinds(x) | |||
There was a problem hiding this comment.
I added SparseArrays. prefix to indicate that it is not exported. But maybe we should export it?
There was a problem hiding this comment.
Maybe rename it to storedindices. See: #33054 (comment)
| * `SparseMatrixCSC` and `SparseVector` are refactored to use accessor functions. It is now | ||
| highly recommended to use accessor methods like `size`, `nzrange`, `rowvals`, `nonzeros`, | ||
| and `nonzeroinds` instead of directly using field (e.g., `A.m`) whose names are | ||
| implementation details ([#32953]). |
There was a problem hiding this comment.
I don't think we can say that the fields and their types of SparseMatrixCSC are implementation details. Many algorithms require explicit modifications to e.g. the colptr and the usage of this is spread far and wide throughout the ecosystem and the fields are in fact documented. If people want to support AbstractSparseMatrixCSC then they would need generic code, but I really think we should try to keep existing code that currently works with SparseMatrixCSC to keep working. The current structure of SparseMatrixCSC is very standard and what I blieve the vast majority of users need.
There was a problem hiding this comment.
Many algorithms require explicit modifications to e.g. the
colptrand the usage of this is spread far and wide throughout the ecosystem and the fields are in fact documented.
Isn't dosomething!(getcolptr(A)) just as efficient as dosomething!(A.colptr)?
I really think we should try to keep existing code that currently works with
SparseMatrixCSCto keep working
All SparseMatrixCSC fields are accessible through equivalent AbstractSparseMatrixCSC interface I proposed in #33054. So I think you can write an efficient algorithm on AbstractSparseMatrixCSC which is as efficient as SparseMatrixCSC? Can you comment on them in #33054?
(Sorry, I probably should have merged this to #33054. I thought #33039 may take sometime to be merged so it might make sense to write a NEWS/docs for its predecessor #32953 which is what this NEWS is all about.)
There was a problem hiding this comment.
Whether necessary or not, direct access to sparse matrix fields is both common and documented. Changing their names in a breaking fashion does not seem viable for 1.x — more attention should have been paid to these names/APIs before 1.0, but it’s too late for that now.
One way forward is to change the field names to better ones and use property overloading to keep code using the legacy names working.
There was a problem hiding this comment.
(For people finding this issue now, the discussion of the assessor API is happening in #33054 and the discussion of the future deprecation strategy is in #33056)
|
Nobody is proposing changing the field names in this PR or any other PR. There is no change to the internal structure, and all the code will keep working as is. The only thing we are doing is encouraging the use of accessor functions. The only thing being figured out is the accessor API. @tkf I am reopening this and we'll figure out the right wording. |
This is a followup of #32953.