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

Export AbstractSparseMatrixCSC + interface #395

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

j-fu
Copy link

@j-fu j-fu commented Jun 1, 2023

See discussion in #265

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.07%. Comparing base (4fd3aad) to head (a602de5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #395   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files          12       12           
  Lines        9188     9188           
=======================================
  Hits         7725     7725           
  Misses       1463     1463           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/sparsematrix.jl Outdated Show resolved Hide resolved
@j-fu
Copy link
Author

j-fu commented May 29, 2024

Similar question for sparse(m::AbstractSparseMatrixCSC). In 1.10 it returns a copy(m). Which may be reasonable. One could argue as well (I tend to prefer this) that the return value should be a SparseMatrixCSC like with the other methods of sparse.

@ViralBShah
Copy link
Member

Generally, the thought with sparse has been that it shouldn't be seen as SparseMatrixCSC specific.

@ViralBShah
Copy link
Member

My concern with exporting was that wouldn't we better off with something more general? AbstractSparseMatrixCSC feels too specialized a thing as an abstract type. But of course, it isn't a huge change per se.

@rayegun Any thoughts?

@ViralBShah
Copy link
Member

Pinging @KristofferC and @fredrikekre on any thoughts here. Another set of eyes would be good here. While I don't see a major issue, a public API would be a big commitment going forward, and AbstractSparseMatrixCSC feels like a bit of a specialized abstract class.

@j-fu
Copy link
Author

j-fu commented Aug 18, 2024

... I'd like to come back to this - Something more general would of course be appreciated, as e.g. discussed in #538 . OTOH, as a matter of fact the interface is used in LinearSolve.jl, see e.g. here and here.
So IMHO it would be good to have this fixed as public API. May be instead of exporting it could be made public now.

@j-fu
Copy link
Author

j-fu commented Aug 20, 2024

@ChrisRackauckas, @oscardssmith, what do you think about this ?

@ChrisRackauckas
Copy link

I think we need to write down what that expected interface is if you're going to consider it public. Like https://docs.julialang.org/en/v1/manual/interfaces/ . Can you try doing a first draft of it?

Maybe @mbauman and @timholy would be good to have on-board as well since they drafted the original array interface and I know they have comments on what a sparse array interface would need to have.

@j-fu
Copy link
Author

j-fu commented Aug 20, 2024

I started a discussion in #559. Not sure if this is the right place. See also #538.

@timholy
Copy link
Contributor

timholy commented Aug 20, 2024

Do Diagonal, Bidiagonal, Tridiagonal, and SymTridiagonal all support this interface? They're examples of sparse matrices even if they are not subtypes of AbstractSparseMatrix. Would a CSR matrix be expected to support getcolptr? What would that even mean?

The deepest thinking I've done on a real interface for sparse arrays of all kinds is in https://github.com/timholy/ArrayIteration.jl (unfinished and effectively abandoned for lack of time). That interface is not so tightly coupled to a particular internal representation. A good start is to ask yourself the question, "what is needed to write algorithms on sparse matrices even if I don't know how they are stored?"

@j-fu
Copy link
Author

j-fu commented Aug 21, 2024

... Certainly, as CSR matrix should have getrowptr and not getcolptr...
Very interesting discussion in ArrayIterators.jl !

I see two strands of discussion here (as I tried to outline in #559):

  • "what is needed to write algorithms on sparse matrices even if I don't know how they are stored?" -- which needs time (which I might not have as well...) and dedication
  • Low level interfaces for coupling to all kinds of (often binary) packages which rely upon the concrete CSC or CSR structure. In this context, the AbstractSparseMatrixCSC interface (getcolptr etc. ) is currently used as a matter of fact though it is not exported or documented as such.

IMHO, both levels of interfaces should (and can) coexist.

@ViralBShah ViralBShah marked this pull request as draft November 2, 2024 17:16
@j-fu
Copy link
Author

j-fu commented Dec 5, 2024

I guess mentioning "interface" in the title of this PR triggered a rather general discussion about what is the right "interface" in the sense of "interfaces". Due to lacking time and resources (including mine...), this discussion is hard finish. Another reason for this IMHO is the lack of formal description tools for interfaces, and we might have to wait for their emergence.

However my main concern is much more pragmatic: I just think that AbstractSparseMatrixCSC and getcolptr should be exported or made public.
So in this context, the title of this PR should have been "Export AbstractSparseMatrixCSC + getcolptr" and avoid mentioning "interface".

Once we have "public" now, I foresee, that the next generation of tools like Aqua will flag many usages of AbstractSparseMatrixCSC and getcolptr e.g. in LinearSolve.jl and the like. Also, in order to give LinearSolve the full power, packages defining preconditioners etc. also might be made aware of AbstractSparseMatrixCSC. I am considering to do corresponding PRs to preconditioning packages, but it feels a bit awkward to propose to use some unexported type and method.

To continue, I see two ways: rename this PR, or open another one which just adds the export statements and some documentation, without mentioning "interface".

j-fu added 2 commits December 5, 2024 13:06
Don't implement any new functionality.
Just describe which methods are expected to be implemented.
@j-fu
Copy link
Author

j-fu commented Dec 5, 2024

I removed any method overwrites, it is just export and docs now.

@j-fu j-fu marked this pull request as ready for review December 5, 2024 22:27
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.

5 participants