Opened 6 years ago
Closed 6 years ago
#18982 closed enhancement (fixed)
New nonexistence tests for strongly regular graphs
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.9 
Component:  graph theory  Keywords:  
Cc:  dimpase  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Jori Mäntysalo 
Report Upstream:  N/A  Work issues:  
Branch:  9d85b97 (Commits, GitHub, GitLab)  Commit:  9d85b9778bbab21e37c429c6923acc6d37af69bf 
Dependencies:  #18960  Stopgaps: 
Description (last modified by )
This branch adds more infeasibility tests from the following paper:
http://www.win.tue.nl/~aeb/preprints/srgsurvey.pdf
Nathann
Change History (12)
comment:1 Changed 6 years ago by
 Branch set to u/ncohen/18982
 Commit set to 6d726758fa86d9b9b4b875ff9cc3ffc183f1bda3
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit changed from 6d726758fa86d9b9b4b875ff9cc3ffc183f1bda3 to d9bb7976aa6fb16259de13c63b54c719f62ae56c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d9bb797  trac #18982: New nonexistence tests for strongly regular graphs

comment:3 Changed 6 years ago by
 Commit changed from d9bb7976aa6fb16259de13c63b54c719f62ae56c to 938c769190182497358f6261ed24279278a533c8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
938c769  trac #18982: New nonexistence tests for strongly regular graphs

comment:4 followup: ↓ 6 Changed 6 years ago by
 Status changed from needs_review to needs_work
Needs rebasing again.
Also when I did raise SomeError('An error occurred.')
I got a comment that it should be raise SomeError('an error occurred')
. (Which seems stupid to my eyes, but is in some PEP.)
comment:5 Changed 6 years ago by
 Commit changed from 938c769190182497358f6261ed24279278a533c8 to 9d85b9778bbab21e37c429c6923acc6d37af69bf
Branch pushed to git repo; I updated commit sha1. New commits:
9d85b97  trac #18982: Merged with 6.9.beta1

comment:6 in reply to: ↑ 4 ; followup: ↓ 8 Changed 6 years ago by
Needs rebasing again.
Done.
Also when I did
raise SomeError('An error occurred.')
I got a comment that it should beraise SomeError('an error occurred')
. (Which seems stupid to my eyes, but is in some PEP.)
I do not see the point of that either. I hope that your reviewer will not stop here then :p
Nathann
comment:7 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:8 in reply to: ↑ 6 Changed 6 years ago by
Replying to ncohen:
I hope that your reviewer will not stop here then
:p
This is free for Somebody Other(tm) to review... I will add my name to reviewersfield if I make significant progress with reading this.
comment:9 followup: ↓ 11 Changed 6 years ago by
 Reviewers set to Jori Mäntysalo
 Status changed from needs_review to positive_review
Well, was not that hard.
Assuming other parts work, this seems to be direct (and correct) translation of equations from the paper to the code. I read it, and tested with examples given in the paper. I think that l
as a variable name makes sense here, because lambda
is already reserved name (and we do not use Fortress language :=)
), even if it violates PEP 0008 part "Names to Avoid". Hence I mark this as positive review.
Btw, graphs.strongly_regular_graph(324,57,0,12)
says "  graph exists.Comments:  ", i.e. has a missing space. You may want to correct it, when next time modifying this file.
comment:10 Changed 6 years ago by
 Description modified (diff)
yeah, lambda
as a variable name might get you confused...
PS. The tests in the reference are hardly new, I changed wording there.
comment:11 in reply to: ↑ 9 Changed 6 years ago by
Well, was not that hard.
Thanks for the review!
Btw,
graphs.strongly_regular_graph(324,57,0,12)
says "  graph exists.Comments:  ", i.e. has a missing space. You may want to correct it, when next time modifying this file.
Done in #19019.
Nathan
comment:12 Changed 6 years ago by
 Branch changed from u/ncohen/18982 to 9d85b9778bbab21e37c429c6923acc6d37af69bf
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #18948: Strongly Regular Graphs database
trac #18948: Two missing graphs
trac #18948: Merged with 6.8
trac #18960: Strongly Regular Graphs from twoweight codes
trac #18960: Merge 6.9.beta0
trac #18960: Adding nonzero somewhere
trac #18960: Again...
trac #18960: Lint > vLint
CURRENT
trac #18982: New nonexistence tests for strongly regular graphs