GraphBLAS / LAGraph

This is a library plus a test harness for collecting algorithms that use the GraphBLAS. For test coverage reports, see https://graphblas.org/LAGraph/ . Documentation: https://lagraph.readthedocs.org
Other
229 stars 61 forks source link

LAGraph_Vector_to_dense produces incorrect result with SuiteSparse v3.3.0draft5 #92

Closed marci543 closed 4 years ago

marci543 commented 4 years ago

I added test for LAGraph_Vector_to_dense in #91.

The function produces incorrect result if it is built with SuiteSparse v3.3.0draft5. Some non-zero values are missing. (test code)

  6x1 GraphBLAS uint64_t vector, sparse by col:
  v, 3 entries
    (0,0)   2
    (1,0)   3
    (4,0)   1

  6x1 GraphBLAS uint64_t vector, sparse by col:
  v_dense, 6 entries
    (0,0)   2
    (1,0)   0
    (2,0)   0
    (3,0)   0
    (4,0)   0
    (5,0)   0

  6x1 GraphBLAS uint64_t vector, sparse by col:
  v_dense_ref, 6 entries
    (0,0)   2
    (1,0)   3
    (2,0)   0
    (3,0)   0
    (4,0)   1
    (5,0)   0

Test log: https://travis-ci.com/github/szarnyasg/LAGraph/jobs/345416111#L27837

With v3.2.2 the problem is not present (branch). Test log: https://travis-ci.com/github/szarnyasg/LAGraph/jobs/345421717#L26747

Can I ask for some advice, @DrTimothyAldenDavis?

marci543 commented 4 years ago

The test code is here: https://github.com/szarnyasg/LAGraph/blob/bcc32554a9d27873461151d5f56b3623f74b115b/Test/VectorToDense/vector_to_dense_test.c

DrTimothyAldenDavis commented 4 years ago

The test passes in the current (master) version of GraphBLAS, with COMPACT turned off, and I get the expected result. The current version isn't much different than the 3.3.0draft5, however.

If I turn off -GBCOMPACT in 3.3.0draft5, I get the right result. If I enable -DGBCOMPACT=1 (as done in the travis tests I think), then I can replicate the wrong answer you're seeing. So this is an issue with the "COMPACT" option in 3.3.0draft5.

DrTimothyAldenDavis commented 4 years ago

Found it. It was a bug introduced when handling the new complex data types in the C=scalar assignment, which worked for the complex case but then broke the compact case. I'll tag 3.3.0draft6 shortly, which fixes this issue.

marci543 commented 4 years ago

Thanks for the fast solution.

DrTimothyAldenDavis commented 4 years ago

And thanks for the fast bug report. I ran the code (with the bug) under my brutal 100% statement coverage tests in GraphBLAS/Tcov ... and all the tests passed, because it only shows up with COMPACT (or with user defined types). I usually also rerun those tests with COMPACT enabled, before the final release, but had I missed that I would have published (gasp!) a bug in the stable version of the code.

I'll be sure to check this condition in my brutal tests.

On Mon, Jun 8, 2020 at 11:00 AM Márton Elekes notifications@github.com wrote:

Thanks for the fast solution.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/GraphBLAS/LAGraph/issues/92#issuecomment-640720062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYIIOJJ4YJOIQZQA2TX54TRVUDKFANCNFSM4NWGCQXQ .