-
Notifications
You must be signed in to change notification settings - Fork 77
RCC, FastAssign, and SwapEdges #310
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
RCC, FastAssign, and SwapEdges #310
Conversation
Merged temporary Branch
| )) ; | ||
| LAGRAPH_TRY (LAGraph_New (G_new, &A_new, LAGraph_ADJACENCY_DIRECTED, msg)) ; | ||
| LG_FREE_WORK ; | ||
| return (num_swaps >= totSwaps)? GrB_SUCCESS : LAGRAPH_INSUFFICIENT_SWAPS ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined LAGRAPH_INSUFFICIENT_SWAPS as 2100 in LAGraphX.h. Was this correct?
Should I return the number of swaps I made? As its own output variable, or do I make totSwaps input/output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reluctant to add a new enum value to the LAGraph return values that is unique to just one algorithm. How about returning LAGRAPH_CONVERGENCE_FAILURE instead? The method iterates, trying to find swaps, and if it doesn't succeed, it does need to return an error.
The description of LAGRAPH_CONVERGENCE_FAILURE is quite similar, "An iterative process failed to converge to a good solution".
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning an error is reasonable. Although currently, I return the error, but still return a valid G_new graph with however many swaps I managed to get before going under the per-loop threshold. Is this a reasonable thing to do, or does returning an error imply the output graph should be freed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Maybe we should talk it over. In your current design, LAGRAPH_INSUFFICIENT_SWAPS is a warning, not an error, right?
src/utility/LG_internal.h
Outdated
| #if GxB_IMPLEMENTATION >= GxB_VERSION (10,0,0) | ||
| #define USING_GRAPHBLAS_V10 1 | ||
| #else | ||
| #define USING_GRAPHBLAS_V10 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this #define is internal, but accessible to any LAGraph algorithm, let's rename it to "LG_SUITESPARSE_GRAPHBLAS_V10". That way, it mimics the LAGRAPH_SUITESPARSE definition (which is in LAGraph.h), but it has an LG_* prefix which means it's for internal use only.
|
Is this ready to merge? |
|
Ready to go |
5eee3cd
into
GraphBLAS:with_GraphBLAS_v10
|
Looks great. Merged into the with_GraphBLAS_v10 branch. |
LAGraph_RichClubCoefficient.c, rcc_demo.c, test_RichClubCoefficient.c, and LG_check_RCC.c
LAGraph_FastAssign.c and FastAssign_demo.c
LG_CC_FastSV7_FA.c, cc_demo.c, and test_ConnectedComponents.c
LG_CC_FastSV7.c and LAGraph.h
LAGr_SwapEdges.c, LAGraph_SwapEdges.c, test_SwapEdges.c, and SwapEdges_demo.c