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
Speedup in GenericGraph.relabel() and two new options #14000
Comments
Attachment: trac_14000.patch.gz |
comment:2
Nathann, you need to put in some tests that show how the code works with the new inputs set to False. Also, please post a timing comparison before and after the patch. Thanks, Anne |
comment:3
Well... The code works the same way. It just does not check some things internally... The output does not change Nathann |
comment:4
I know, but it is still good to put in a test that checks that it works the same ;-) Replying to @nathanncohen:
|
comment:5
Yep yep I'm doing it right now, but the point is that I am not totally sure that it does not produce a memory leak somewhere Nathann |
Attachment: trac_14000-doctests.patch.gz |
comment:6
A timing, just for the show : Before :
After :
Ready for review again ! Nathann |
This comment has been minimized.
This comment has been minimized.
Author: Nathann Cohen |
Changed keywords from none to relabelling |
comment:8
Replying to @nathanncohen:
What does this mean? Do you mean disabling the option is at the user's own risk? |
comment:9
Precisely ! I wrote that in the doctest itself : "Checking that all vertices have an image can require some time, and this feature can be disabled (at your own risk)" If you "rename" the vertices of a graph in such a way that two vertices end up having the same name at the end of the procedure, I can totally imagine that some data will be lost on the way Nathann |
comment:10
Ok, then I am happy with your patch! Anne |
Reviewer: Anne Schilling |
comment:13
Cool. Thanks for reviewing it ! Nathann |
This comment has been minimized.
This comment has been minimized.
Merged: sage-5.8.beta2 |
Because of some weird graphs built by ennemies of The Good Graph People, it seems that most of the work done by GenericGraph.relabel() is totally useless.
This patch adds two optional arguments to this method, which let the users enable/disable tests.
Apply:
CC: @stumpc5 @fchapoton
Component: graph theory
Keywords: relabelling
Author: Nathann Cohen
Reviewer: Anne Schilling
Merged: sage-5.8.beta2
Issue created by migration from https://trac.sagemath.org/ticket/14000
The text was updated successfully, but these errors were encountered: