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

Remove csc_permute and ereach from base/sparse/csparse.jl #12231

Closed
dmbates opened this issue Jul 20, 2015 · 15 comments
Closed

Remove csc_permute and ereach from base/sparse/csparse.jl #12231

dmbates opened this issue Jul 20, 2015 · 15 comments
Labels

Comments

@dmbates
Copy link
Member

dmbates commented Jul 20, 2015

The csc_permute and ereach functions defined in base/sparse/csparse.jl are not exported and, I expect, not used. I have an unregistered CSparse.jl package which I will move to the JuliaSparse group. When that package is registered I suggest that those functions be removed, or if that is too drastic, deprecated then removed.

@kshyatt kshyatt added the domain:arrays:sparse Sparse arrays label Jul 20, 2015
@tkelman
Copy link
Contributor

tkelman commented Jul 20, 2015

Would be best to check whether there are packages currently using that functionality. If there are, I'd rather go with deprecation prior to 0.4 (if you're in a hurry) and/or removal afterwards.

@tkelman
Copy link
Contributor

tkelman commented Oct 14, 2015

@KristofferC are you using csc_permute anywhere? You touched it in #12734, any objections to moving it somewhere else, e.g. https://github.com/dmbates/CSparse.jl?

@KristofferC
Copy link
Sponsor Member

I don't use it. I just saw it was untested and found a bug when I added a test for it. Fine by me to move.

@Sacha0
Copy link
Member

Sacha0 commented Jan 27, 2016

What are current plans for etree, ereach, symperm, and csc_permute? I am contemplating whether I should replace anything further from base/sparse/csparse.jl after #14702 and #14798, and if so what next. csc_permute should be a short child of qftranspose!, though with multiple dispatch permute(A::SparseMatrixCSC, pinv, q) seems better name-wise. symperm could also be a short child of qftranspose!, though I imagine more efficient approaches to symmetric permutation exist. Thanks, and best!

@tkelman
Copy link
Contributor

tkelman commented Jan 27, 2016

symperm and etree are exported, documented and probably more useful than the other two.

It's an outstanding TODO to move SuiteSparse bindings out to a package for licensing (among other) reasons, and I think when that happens it would be natural to move at least the unexported functions (and possibly the exported ones too) at the same time.

@KristofferC
Copy link
Sponsor Member

What is the status on this? Is CSparse.jl ready to be tagged? Now when we have a good sparse function we could probably get rid of all csparse stuff from base julia.

@KristofferC
Copy link
Sponsor Member

cc @dmbates

@dmbates
Copy link
Member Author

dmbates commented Apr 26, 2016

The symperm function can be done more cleanly in Julia using Symmetric or Hermitian types with SparseMatrixCSC as the matrix type. I'm not sure if it should be deprecated before being removed.
I guess it would depend on whether packages use it.

The etree function is a bit trickier. I have no doubt that there are people who could write a Julia implementation without looking at the CSparse code but I am not such a person. I believe etree is available in Matlab and might be missed by some users if it was not available. I think it would be best if someone could write a native Julia implementation.

@Sacha0
Copy link
Member

Sacha0 commented Apr 26, 2016

Apologies for the radio silence since #15242; I've been focusing on a related project. Writing general and symmetric permutation methods for SparseMatrixCSC remains on my radar. Assuming no code appears in the interim, I plan to give those methods a shot once I have a prototype of the aforementioned project out (in a few weeks). Elimination tree generation is on my horizon as well, though distant.

@KristofferC
Copy link
Sponsor Member

As a first step couldn't they be moved to the csparse package? My hunch is that these methods are not extensively used and if we make sure that it is as simple as a Pkg.add to restore the functionality it would be nice to get rid of one GPL part in Base.

@dmbates what do you think about registering your package and pointing people to that package in the same way we do with PkgDev now?

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2016

LGPL. Doug and git do not seem to get along, so perhaps best if he adds you as a collaborator @KristofferC.

@KristofferC
Copy link
Sponsor Member

I have no problem doing the work needed as long as d people think it is a good idea and @dmbates think his package is ready to be registered. Otherwise I can make a new package which would only contain the functions to be moved from Base.

@dmbates
Copy link
Member Author

dmbates commented Apr 26, 2016

@KristofferC Although I would like to go back and look at that code and update it I am facing a deadline at the end of the week. Experience suggests that it is best for me not to start looking at other code right now.

I have added you as a collaborator on the repository. If you think it is worthwhile registering the package please do so. Next week I will try to look at documentation, etc. for this package. I haven't worked on it for a long time.

@tkelman
Copy link
Contributor

tkelman commented Jun 20, 2016

closed by #17033, thanks again @Sacha0!!!

@tkelman tkelman closed this as completed Jun 20, 2016
@Sacha0
Copy link
Member

Sacha0 commented Jun 20, 2016

Thanks to @KristofferC for doing the work rather :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants