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

Make SparseArrays.jl a weak dependency #885

Closed
wants to merge 2 commits into from
Closed

Conversation

dkarrasch
Copy link
Member

This is a little weird: one could get a sparse matrix just by using a specific keyword argument, without ever calling or loading SparseArrays.jl. So I'm not sure to which extent this might be considered breaking. In terms of future perspective, this is very desirable for downstream packages to not load SparseArrays.jl unconditionally, especially on v1.10+.

@devmotion
Copy link
Member

This seems very breaking to me: It breaks indicatormat(...; sparse=true) for everyone that has not loaded SparseArrays (im- or explicitly). It seems generally weird to me that this PR means that one branch of indicatormat would be broken by default.

So I think SparseArrays should be kept as a regular dependency or the design of indicatormat should be changed (e.g., it could always return a dense indicator matrix and the for the sparse version a separate function spindicatormat would be defined, with a stump in the main package and a proper implementation in the extension (which arguably is a bit weird and not the prototypical dispatch-based example for an extension - maybe a dispatchable type as optional argument would be better?)).

@dkarrasch
Copy link
Member Author

Ok, I'll close this PR then, but not without re-iterating that a dependency on SparseArrays.jl should be, in the future, reviewed very carefully, especially for fundamental packages like StatsBase.jl. 😉

@dkarrasch dkarrasch closed this Aug 23, 2023
@dkarrasch dkarrasch deleted the dk/project_cleanup branch August 23, 2023 11:57
@aplavin
Copy link
Contributor

aplavin commented Dec 9, 2023

Such a dependency understandably didn't/doesn't seem an important decision simply becase SparseArrays are included into Julia and its default sysimage. Also, it still is a dep of Statistics.jl.

But this is changing, as I understand SparseArrays won't be in sysimage in 1.11 (making loading time worse), and Statistics won't depend on it either. Would be useful to reconsider this dependency here as well, whether it's worth keeping it for such an obscure piece of functionality: I don't see any code on github that uses indicatormat(...; sparse=true) (search link).

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.

3 participants