Fix issues with A_mul_Bc and Ac_mul_B for CholmodSparse (#9160).

[acroy]

As described in the title. Is there any test for Cholmod related functions?

[andreasnoack]

Thanks. Could you add some tests for the cases you described in #9160?

[tkelman]

https://github.com/JuliaLang/julia/blob/master/test/sparse.jl is as good a place to put them as any, a little easier than making a new file under test/linalg/. If you could label the tests with a comment that refers to the issue number that would be awesome. More test coverage is always welcome, especially for sparse functionality where the current tests can miss a few problems as you've seen.

[andreasnoack]

I'd prefer if they would be places in a test/linalg/cholmod.jl file. I think it would be easier to track test coverage if the tests are put in a file of the same name of the source.

[tkelman]

That could work as long as it's added to the linalg list in test/runtests.jl, though currently the tests under the linalg folder are a little harder to run individually - you can do make test-sparse, or make test-linalg to run all of the linalg group, but not for the individual files under test/linalg/. Maybe it could work by adding another complicated $(subst .jl,,$(wildcard *.jl)) thing in test/Makefile.

[acroy]

Sure. I will add some tests in test/linalg/cholmod.jl and then we can see how it works out.

[acroy]

I put them in test/linalg/cholmod.jl and added this to the linalg tests in test/runtests.jl. Should we test Float64 and Float32 elements?

[andreasnoack]

Great. Preferably, we should test all four data types supported by CHOLMOD.

[acroy]

I will have a look at the trailing whitespace. I got a similar error on my Mac if I try to use Float32s. Looking closer at this I get "CHOLMOD ERROR: sparse: float unsupported" if I try to create CholmodSparse. I am not sure what is going on ... Even worse for complex entries I get a segfault even for the normal *. I will have to look at the docs again ...

[acroy]

Ok the docs for cholmod_ssmult says

Only pattern and real matrices are supported. Complex and zomplex matrices are supported only when the numerical values are not computed (values is FALSE).

So complex matrices shouldn't be allowed for *.

[andreasnoack]

It appears that the new tests are very useful.

[acroy]

Seems that the Float32 issue depends on the platform. On my Mac I can't construct CholmodSparse with Float32 entries, while it works on another Linux machine. I modified the tests. Hopefully travis and/or appveyor give us some clues.

[tkelman]

Evidently suitesparse is a mess. Looks like there was already a https://github.com/JuliaLang/julia/blob/master/test/suitesparse.jl file that these would be better for IMO. Or we move and/or split up the contents that are in that file into smaller files in linalg.

[andreasnoack]

Yes. If any of the tests in suitesparse.jl are actually tests of the definitions in base/linalg/cholmod.jl, I think they should just be moved to the new file.

[acroy]

Looking into the future and since the linalg tests are already so heavy, what about having a directory test\sparse similar to test\linalg? There we could put cholmod.jl and umfpack.jl etc.

@tkelman : Do you have a good guess what the errors on windows are about?

[andreasnoack]

As mentioned, I think that we should work towards a structure where the test files corresponds to the files in base. Some of the confusion with the sparse stuff is that we both have the base/sparse directory and the base/linalg/sparse.jl, -unfpack.jl and -cholmod.jl files. I'd be okay with collecting all sparse definitions to the sparse directory. We should also hear what @ViralBShah has to say here.

[tkelman]

@acroy no idea. I get the same CHOLMOD error: F is too small on win32 for the very first call to CholmodSparse(A), with Ti = Int64; elty = Float64.

[acroy]

What the F is F? It seems this error is generated in cholmod_transpose.c. Very mysterious.

[ViralBShah]

I am OK with having all sparse stuff in one directory, including linear algebra.

[acroy]

For some reason Cholmod on my Mac doesn't support Float32s. The Travis build is apparently fine though. [EDIT: I have to take that back. There was a typo in my test. Cholmod with Float32 doesn't work.]

@ViralBShah : This should concern cholmod.jl, cholmod_h.jl, umfpack.jl, umfpack_h.jl and spqr_h.jl (the latter seems to be unused) in linalg/. But it might be better to move them in a separate PR?

[acroy]

I am a bit confused, but maybe this is as simple as that: in the documentation (section 8, first paragraph) it says

CHOLMOD supports both int and long integers. CHOLMOD routines with the prefix `cholmod_`
use `int` integers, `cholmod_l_` routines use `long`. All floating-point values are `double`.

Although there is a constant (CHOLMOD_SINGLE) for dtype which suggests also floats are allowed, in cholmod_core.h there is a comment (just below the constant definition) saying

 FUTURE WORK: the float case is not supported yet.

So I guess that means that we have to remove Float32 from the list of supported types?

[ViralBShah]

Interesting. I did not know that. We should remove Float32 support then, I guess.

[acroy]

The tests on windows are still failing. The Travis linux build fails for linalg/pinv. Is that unrelated?

[andreasnoack]

The pinv error must be unrelated. I think F is the result array when transposing. I don't know how it is allocated and why it is Windows specific.

[tkelman]

I think the pinv tolerances are a little too tight, that's an intermittent failure I've seen once in a while.

Not sure exactly where the failure is happening, but maybe we can try to create a standalone C reproduction case to see if it's an upstream bug? I don't know if SuiteSparse has a mailing list or issue tracker of any kind though.

[andreasnoack]

@tkelman Yes, Iagree. The tolerance should be adjusted a bit, but it shouldn't be a concern for this PR.

[acroy]

@tkelman Maybe the demos in deps/SuitSparse-4.x.x/CHOLMOD/Demo would work?

[tkelman]

I meant a reproduction of how we're triggering this error. Here's a better backtrace on release-0.3 https://gist.github.com/tkelman/18b8226652251afc7bee showing that it seems this was already broken and untested, your changes don't appear to have made anything worse at least.

[tkelman]

Is there a way to do this without eval? Put it inside the if perhaps?

[acroy]

Sure, I can put the typealias inside the if. symbol(::Type{T}) would be really handy.

[tkelman]

This worked for me on win32 and win64 without the changes to deps/Makefile. I'd rather avoid tweaking it if it's not necessary.

@andreasnoack and @dmbates could you look over the changes to base/linalg/cholmod.jl?

[andreasnoack]

This looks odd. Why the Union?

[acroy]

Just for symmetry reasons :-) I can remove the Unions if you want.

[andreasnoack]

I think it look odd, but I don't think it matters.

[andreasnoack]

As far as I can see, the changes are good.

[andreasnoack]

Is this ready for merging?

[tkelman]

I think so. It will break AppVeyor for as long as it takes to rebuild a new nightly with updated suitesparse wrapper. We might need to manually clear out the buildbot's copy.

[andreasnoack]

@tkelman Is there a way to avoid that? If not then let's go ahead and merge.

[tkelman]

We can ask @staticfloat to manually build a nightly from this branch and make sure it gets pushed to status.julialang.org first? I do have a login to the buildbot but I'm not sure how I would go about doing that yet.

[staticfloat]

It should take about 45 minutes to an hour for your merge here to result in a new build available on status.julialang.org, so I say go ahead and just merge it. If you need to clean the deps, you can do so by logging in to the buildbot and using the new "clean arpack/openblas/suitesparse" force builds available from this page. So just run that right before you merge this, and you should be good to go.

[tkelman]

Awesome, thanks for adding the cleaners. Based on some local testing, I think the usual make cleanall that should be happening already will be good enough to catch this one. Let's merge and see.

[acroy]

Thanks everyone! Do you think that this is worth to be back ported to 0.3?

[tkelman]

I think so, since it was pretty buggy before. @JuliaBackports @ivarne what's your opinion?

I suspect since the linalg tests have been rearranged on master it won't be a clean cherry-pick, could you prepare it as a new PR against release-0.3? And since the buildbots don't make new binaries off of the release branch all that often, I'll probably want to tweak appveyor to recompile just the suitesparse wrapper.

[tkelman]

@staticfloat the ubuntu and centos builders look stuck in a "Retry exception slave lost" death loop? Funny how I previously wanted to make sure release-0.3 manual builds wouldn't get uploaded as nightly, now I do want a manually-triggered nightly...

[ivarne]

git cherry-pick -x -e won't be able to help you much in the process of backporting this (unless you are using some crazy tricks). The Uint->UInt rename will be an effective blocker for quite a few lines. If a PR shows up (without UInts), I'm generally in favor of backporting as much as possible, while keeping the BC promise. Sparse linear algebra is not the area where you should listen to me though.

[tkelman]

Sometimes you can do curl https://github.com/JuliaLang/julia/pull/9189.patch | sed 's/UInt/Uint/g' | git apply, but that won't quite work here. It'll need to be done manually.

[tkelman]

backported in c5158b8, pretty sure I resolved the conflicts right (passed tests on linux32 and win64, checking appveyor and travis now for the rest)