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

support user-specified permutation in CHOLMOD factorizations #10844

Merged
merged 1 commit into from
Apr 16, 2015

Conversation

stevengj
Copy link
Member

This changes the cholfact and ldltfact sparse factorizations (via CHOLMOD) to accept a perm argument specifying a user-defined permutation instead of the default AMD permutation. It also changes the (undocumented) optional β argument to a (documented) shift keyword argument. Some of the code is consolidated a bit, and I switched to using the new-style Ref{Complex128} instead of Ptr{Cdouble} (which seems easy to misuse by passing only a single real number when CHOLMOD sometimes expects two) for a couple of ccalls.

I included a couple of other keywords args for cholmod options I was experimenting with, but was unsure whether to document them. Passing perm and setting userperm_only=false causes CHOLMOD to try both the user-specified permutation and AMD and pick the better of the two, and passing postorder=false omits a post-ordering pass in CHOLMOD (that doesn't affect nnz but supposedly effects supernodal efficiency).

cc @andreasnoack, who helped track down the relevant functionality (I needed it for teaching, in order to show the effect of different orderings).

@stevengj stevengj added the sparse Sparse arrays label Apr 16, 2015
@andreasnoack
Copy link
Member

👍

@stevengj
Copy link
Member Author

See also this IJulia notebook from my class.

…β optional argument to a shift keyword arg, some code consolidation; change & to new Ref syntax in cholmod
@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2015

Ptr{Cdouble} (which seems easy to misuse by passing only a single real number when CHOLMOD sometimes expects two) for a couple of ccalls.

at no point should it have been valid to pass Ptr{Cdouble},& where a Ptr{Complex128},& was intended. the result should have been incorrect answers and corruption of the program. was that what was being done here previous?

@ViralBShah
Copy link
Member

👍

@andreasnoack
Copy link
Member

@vtjnash I think the old version was wrong. The signature for the C function is double alpha [2] and I think I simply missed the [2] part. It might be that only real arguments are tested for this function, but it is also a bit tricky because it seems that the it has to be a genuinely complex argument to cause trouble, e.g.

julia> SparseMatrix.CHOLMOD.sdmult!(AS, false, complex(1.0), complex(0.0), bS, cS)

works, but

julia> SparseMatrix.CHOLMOD.sdmult!(AS, false, complex(1.0, 1.0), complex(0.0), bS, cS)
ERROR: InexactError()
 in ptr_arg_cconvert at base.jl:58
 in sdmult! at sparse/cholmod.jl:578

Anyway, @stevengj has fixed the problem.

@stevengj
Copy link
Member Author

@vtjnash, in at least some cases, CHOLMOD was only reading beta[0] even though the argument was declared as double beta[2].

stevengj added a commit that referenced this pull request Apr 16, 2015
support user-specified permutation in CHOLMOD factorizations
@stevengj stevengj merged commit 432f440 into JuliaLang:master Apr 16, 2015
@stevengj stevengj deleted the cholmod_userperm branch April 16, 2015 12:43
@stevengj
Copy link
Member Author

We might want a backport of the complex-arg fix, but I don't think the API changes should be backported.

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

Successfully merging this pull request may close these issues.

4 participants