-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
Computation of rank-decompositions in Sage #11754
Comments
Reviewer: David Coudert |
comment:2
Nathann, I tried to install the patch with sage-4.8.alpha5 and it's not possible anymore. Best,
|
comment:3
I thought it would be much worse ! Just obvious conflicts with the vertex_separation stuff... Patch updated ! Nathann |
comment:4
I can now install the patch correctly.
|
comment:5
HMmmmmmm.... In the part of the code I wrote the memory allocations are checked, and it would be bad if it came from the original source code. We try not to put our hands in this kind of code, in order to avoid any troubles when the original source codes get updated (we would have to do all our modifications again on the updated version). What we usually do in these cases is report the bug upstream and wait for them to fix it. The thing is that I was not able to reproduce your bug on my computer. It begins to eat a lot of memory but no segfault, so I do not really know how to debug it Are you testing this code on your mac ? I hope it is not one of those integer initializations again Nathann |
comment:6
The test has been done on a standard PC (32 bits) with 4G of RAM and running linux. Nothing special. The error is almost instantaneous. Some simple tests.
Testing with 32 nodes => the patch is working for 31 vertices only !
The patch contains some test regarding memory requirement
But the test is not always working, perhaps because I'm on a 32 bits computer...
I also have a segfault when running on an empty graph. A simple test is certainly missing.
Hope it helps fixing the problems. Best, David. |
comment:7
Hellooooooo David !! I just updated the patch so that the case n == 0 is checked. This being said, there is nothing I can do with the segfault you meet as I have not been able to reproduce it. I read my code several times and try to pay attention to those memory errors, all the malloc are checked, etc... I am beginning to think the error may be in the original C code and perhaps it is not too hard to spot by adding "printf("Hello\n");" at some different places until we know where the error is coming from. The thing is that I really can not debug this without being able to reproduce the bug. I tried maaaaaaany times t create a random graph and give it a try but it never happened. Did you tell me that the error occurred right when the function was called ? And do you know if it happens "often" ? How many random graphs do you try before it segfaults ? Nathann |
comment:8
I just tried the new version of the patch. It is now OK for empty graphs
I don't know what is the expected result for non-connected graphs. I have the following result:
Now, concerning large graphs:
I tried with other 30 vertices graphs: G = graphs.GridGraph([5,6]), G = graphs.RandomTree(30), G = graphs.CompleteGraph(30), ... and I have exactly the same segfault. The problem comes from the "if sage_graph_to_matrix(G)" in which you use the " int init_rw_dec(uint_fast8_t n) " function that uses the "int init_rw(uint_fast8_t n)" function. Best, |
comment:9
Helloooooo !!! Patch updated to make the 32 limit clearer About the segfault : the point is that there is nothing I could "check" to return an exception to avoid the segault unless I know where it comes from. The function you mention contains a total of 5 lines which can produce no segfault, and exclusively consist in memory allocations and checking the memory has been allocated
If anything goes wrong, the function return -1 instead of 0. And this function (which is defined in the original C code) is called as you said, by a "if" that is precisely there to ensure the memory allocation went smoothly. If it did not, it also returns and error and the function above (rank_decomposition) raises the exception. I mean, there is no memory allocation that I could check and that is not checked already Nathann |
comment:10
I added some printf to track the segfault. It occurs in the calculate_level function for ss=1 and i=17. More precisely, the segfault is with the instruction cslots[1 << i] = 0x0;.
This is definitely a memory allocation problem. D. |
comment:11
Ahah !! i=17 ! Now that's something ! I prefer when it is something around a power of two, it makes more sense Well, for instance I wondered what the result of
is in C. Is 1 automatically set to type uint_fast8 ? In this case the result is zero. Is it automatically set to integer ? In this case the result is 2^17. Anyway this should not make much of a difference, as accessing cslots[0] is not big deal -- it is the safest of them all to access !
Nathann |
comment:12
I tried with a fresh install of 5.0.beta1 and I still have the problem with 30 :( D. |
comment:13
Ok... I gave it another try and really have no idea where this could come from. As the only thing I can guess is that the problem actually come from the original source code, what would you think of merging this patch anyway, with a warning in the documentation (also to ask people experiencing the same bug, if any, to give us information on their hardware), and continue to discuss it with the author ? The way we are going now, this patch will just be forgotten even though it can actually solve the problem except on your architecture for some mysterious reason Nathann |
comment:14
I agree with this proposal, so I give a positive review. Honestly, I think most of us will never try use the algorithm for N=30 due to very huge computation time ;-) D. |
comment:15
Argggg !!! A bit too fast !! The warning was not in the patch when I said that ! Could you check this other patch too ? It contains the message I mentionned and also fixes a warning when building the documentation. Sorryyyyyyyyyyyyyyyyyy !! Nathann |
comment:16
Attachment: trac_11754_warning.patch.gz I tried the warning patch as well, and the documentation is properly generated with a clear message (pink warning box). I give a positive review. D. |
comment:40
Nathann, I have added a review patch because 1) the test in rw.h was not correct. We have to define a tag. 2) the test in rw.c was also incorrect due to missing braces. Now compilation is correct on both my 32 bits and 64 bits computers. The execution is correct, in particular it returns the correct error message when memory is insufficient. Tests are OK, and documentation was already OK. For me the patch is now good to go. D. |
comment:41
Hellooooo !!!
Hmm... That should be fixed in his code. I will email him about it
One last mail exchange with him, and I'll be glad to see this patch merged ! Nathann |
comment:42
I did some more tests with sage-5.0.beta5
Altogether, I need the revision patch (as I already identified weeks ago) to have the patch properly installing and operating. D. |
comment:43
Wysiwyg problem... So change the test from
To
Honestly, which one is the fastest to read ? |
comment:44
Ok, the author told me he intended the priority to be different (a parenthesis around "n && !(sizeof(uint_fast8_t) * (1ul << n))", but it makes no difference anyway as we settle the case n=0 differently already. On my machine everything runs fine with all 4 patches applied. If everything is fine on your computer too I think this patch can go. As for a possible updates in 50 years, we will still have this track ticket lying around Nathann |
comment:45
Attachment: trac_11754-ifndef-rev.patch.gz The author is right ;-) The previous version was also working for me because the test was true for N> 31, n <0, and N=30, but it was an accident. With the correct version, we have the same result but it is more expected. I have updated the patch accordingly and its working fine on all my computers: compilation, tests, documentation, functionality, error messages. For me it's good to go so I give positive review. D. PS: I hope the ARM guys will not experience similar problems than for vertex separation... |
comment:46
Well, at least not "the same ones", as the author uses the correct types. And I'll do that too from now on Thank you for the review, by the way ! This is another of the patches that have been waiting for... ages ! Nathann |
Merged: sage-5.0.beta6 |
comment:48
Re-opening this as there some problems with the
|
comment:49
Helloooo !!! Is this patch what you need ? Nathann |
comment:50
Attachment: trac_11754-RM.patch.gz |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:51
Attachment: 11754_manifest.patch.gz |
Changed reviewer from David Coudert to David Coudert, Jeroen Demeyer |
This comment has been minimized.
This comment has been minimized.
comment:53
I think there is an issue with this code as there is a extension module sage.graphs.graph_decompositions.rankwidth as well as a package (with init.py) sage/graphs/decompositions/rankwidth/ . I think that should really be fixed as it's ambiguous which should be used. |
comment:54
To be fixed in a follow-up ticket of course. |
comment:55
This is now at #12684. |
Hello everybody !
This patch is an interface between Sage and the software RW, written by Philipp Klaus Krause [1]. It is written in C, and freshly licensed under the GPL2. It is a highly enumerative code that uses a lot of memory, but is still so far the best practical way to obtain optimal decompositions.
This patch creates the module sage.graphs.graph_decompositions and the rankwidth-related files. As it requires some documentation, some words are added on what a rank-decomposition is and where the code is coming from. Doing that, I also added sage.graphs.modular_decompositions to the documentation.
(As I created a module graph_decompositions, it would make sense to move te modular_decomposition there. It thought it best to do it in a distinct patch, this one being large enough already)
Oh. By the way, most of the patch actually contains the copy of the files written by Philipp Klaus Krause.
Nathann
http://pholia.tdi.informatik.uni-frankfurt.de/~philipp/software/rw.shtml
APPLY:
CC: @sagetrac-lsampaio @sagetrac-mvngu
Component: graph theory
Author: Nathann Cohen
Reviewer: David Coudert, Jeroen Demeyer
Merged: sage-5.0.beta6
Issue created by migration from https://trac.sagemath.org/ticket/11754
The text was updated successfully, but these errors were encountered: