-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Test, document, and export dropzeros[!] #16947
Conversation
@@ -795,7 +795,24 @@ end | |||
droptol!(A::SparseMatrixCSC, tol, trim::Bool = true) = | |||
fkeep!(A, (i, j, x) -> abs(x) > tol, trim) | |||
|
|||
""" | |||
dropzeros!(A::SparseMatrixCSC, trim::Bool = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the signature should also go in the rst somewhere so make docs
populates the docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry looks like I should've been more specific. linalg is probably not the best place for these, there is a
.. _stdlib-sparse:
Sparse Vectors and Matrices
---------------------------
section in arrays.rst
where these would be a better fit I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's must better! Fixed on push. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also droptol!
for SparseVector
, maybe change SparseMatrixCSC
to SparseArray
in the signatures`?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch re. SparseVector
s! I've updated the pull request with expanded tests and documentation for the SparseVector
dropzeros[!]
methods.
AppVeyor timeout unrelated? |
#16556. LGTM, but will leave open a bit longer for any other comments. |
…nd `SparseVector`s.
Likewise thanks for the review and merge here! |
In #15242/#14798 and related threads, concensus formed in favor of exporting
dropzeros[!]
with the shift to retaining stored numerical zeros inSparseMatrixCSC
s. This pull request more extensively tests, documents, and exportsdropzeros[!]
. (@tkelman, other behaviors I should test?) Best!