Opened 3 years ago

Closed 3 years ago

#18982 closed enhancement (fixed)

New non-existence tests for strongly regular graphs

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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) Commit: 9d85b9778bbab21e37c429c6923acc6d37af69bf
Dependencies: #18960 Stopgaps:

Description (last modified by dimpase)

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 3 years ago by ncohen

  • Branch set to u/ncohen/18982
  • Commit set to 6d726758fa86d9b9b4b875ff9cc3ffc183f1bda3
  • Status changed from new to needs_review

New commits:

a0173e2trac #18948: Strongly Regular Graphs database
4adcf95trac #18948: Two missing graphs
cf50d70trac #18948: Merged with 6.8
6390dd9trac #18960: Strongly Regular Graphs from two-weight codes
3eee1e7trac #18960: Merge 6.9.beta0
606d15ftrac #18960: Adding nonzero somewhere
83ed332trac #18960: Again...
dc2d326trac #18960: Lint -> vLint
4fb3ce1CURRENT
6d72675trac #18982: New non-existence tests for strongly regular graphs

comment:2 Changed 3 years ago by git

  • Commit changed from 6d726758fa86d9b9b4b875ff9cc3ffc183f1bda3 to d9bb7976aa6fb16259de13c63b54c719f62ae56c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d9bb797trac #18982: New non-existence tests for strongly regular graphs

comment:3 Changed 3 years ago by git

  • Commit changed from d9bb7976aa6fb16259de13c63b54c719f62ae56c to 938c769190182497358f6261ed24279278a533c8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

938c769trac #18982: New non-existence tests for strongly regular graphs

comment:4 follow-up: Changed 3 years ago by jmantysalo

  • 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.)

Last edited 3 years ago by jmantysalo (previous) (diff)

comment:5 Changed 3 years ago by git

  • Commit changed from 938c769190182497358f6261ed24279278a533c8 to 9d85b9778bbab21e37c429c6923acc6d37af69bf

Branch pushed to git repo; I updated commit sha1. New commits:

9d85b97trac #18982: Merged with 6.9.beta1

comment:6 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by ncohen

Needs rebasing again.

Done.

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.)

I do not see the point of that either. I hope that your reviewer will not stop here then :-p

Nathann

comment:7 Changed 3 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:8 in reply to: ↑ 6 Changed 3 years ago by jmantysalo

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 reviewers-field if I make significant progress with reading this.

comment:9 follow-up: Changed 3 years ago by jmantysalo

  • 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 3 years ago by dimpase

  • 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 3 years ago by ncohen

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 3 years ago by vbraun

  • Branch changed from u/ncohen/18982 to 9d85b9778bbab21e37c429c6923acc6d37af69bf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.