Opened 7 years ago

Closed 7 years ago

#19224 closed enhancement (fixed)

swtich OA(k,n)+* strongly regular graphs

Reported by: Nathann Cohen Owned by:
Priority: major Milestone: sage-6.9
Component: graph theory Keywords:
Cc: Dima Pasechnik Merged in:
Authors: Nathann Cohen Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 9433564 (Commits, GitHub, GitLab) Commit: 9433564213a8a699814106c375b4cc0a5ef22f71
Dependencies: Stopgaps:

Status badges

Description

This branch adds the "switch OA(k,n)+*" graphs used in Andries Brouwer's database.

Nathann

Change History (18)

comment:1 Changed 7 years ago by Nathann Cohen

Branch: u/ncohen/19224
Commit: 92a2a6fb4bce16667412a5d206133d6128b25c00
Status: newneeds_review

New commits:

92a2a6ftrac #19224: swtich OA(k,n)+* strongly regular graphs

comment:2 Changed 7 years ago by Vincent Delecroix

Status: needs_reviewneeds_work

Hello,

  1. You wrote
    from math import sqrt
    cdef int n = int(sqrt(n_2_p_1-1))
    
    Sage integers do have a method sqrtrem that gives you the square root and the rest
    sage: x = ZZ.random_element(0,10000)
    sage: s,r = x.sqrtrem()
    sage: x == s**2 + r
    True
    
    But as you are using int from Cython you would better use standard C functions
    from libc.math cimport sqrt, floor
    
  1. Why are you testing withtout the option resolvable=True
    if (k % n          or
       ...
       not orthogonal_array(c+1,n,existence=True)):
          return None
    
    while using in the function
    def switch_OA_srg(c,n):
        ...
        OA = map(tuple, orthogonal_array(c+1,n,resolvable=True))
        ...
    
  1. Use better spacings: int v,int k,int l -> int v, int k, int l.
  1. Do you mind moving out the function switch_OA_srg into a global function? With such local definition you are generating again and again different Python functions with the very same code
    sage: f1 = is_switch_OA_srg(290, 136, 63, 64)[0]
    sage: f2 = is_switch_OA_srg(290, 136, 63, 64)[0]
    sage: f1 is f2
    False
    
  1. Could you add an example of a SRG that was not available before?

Vincent

comment:3 Changed 7 years ago by git

Commit: 92a2a6fb4bce16667412a5d206133d6128b25c009fc78a492823ede303f45a4a6238d06a0c20f87e

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

580e422trac #19224: Merged with 6.9.beta7
9fc78a4trac #19224: Review

comment:4 in reply to:  2 ; Changed 7 years ago by Nathann Cohen

Hello,

  1. You wrote
    from math import sqrt
    cdef int n = int(sqrt(n_2_p_1-1))
    

I did that because sqrt(-4) raises an exception, while in C floor(-a) returns nan. I prefer this behaviour when I do not need the speed. As a reviewer, your job is to make sure that the code is correct an reliable, properly documented etc. These kind of comments are to me a matter of taste, and it would be cool if you proposed a commit rather than include as part of your review that the way it is done does not conform to your taste. As there isn't, I believe, any reason to prefer yours over mine in this context where the speed difference does not matter.

  1. Why are you testing withtout the option resolvable=True

That's a mistake indeed.

  1. Use better spacings: int v,int k,int l -> int v, int k, int l.

As previously, please propose a commit the next time you cannot tolerate a missing space.

  1. Do you mind moving out the function switch_OA_srg into a global function? With such local definition you are generating again and again different Python functions with the very same code

It is not that I would mind, but mostly that I would not know how to write its documentation to my taste. The subfunction takes c and n as parameters, whose role has to be explained. Plus 'c' is actually used twice in the function in "different ways" (it has two different meanings in the construction), so that the independent function should probably differenciate the two uses of 'c' instead of tying them to be equal (and I have no use for that generalization). The result would be unclear, look hardcoded, and the name of the function is hard to pick too as this class does not seem very standard.

More technically, I could not care less about returning functions which are not equal to each other. My job here is to build new graphs.

  1. Could you add an example of a SRG that was not available before?

What about the 4 that already appear in the doctest?

Nathann

comment:5 Changed 7 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:6 in reply to:  4 Changed 7 years ago by Vincent Delecroix

Replying to ncohen:

  1. Could you add an example of a SRG that was not available before?

What about the 4 that already appear in the doctest?

I meant in the function graphs.strongly_regular_graphs.

Vincent

comment:7 Changed 7 years ago by Nathann Cohen

Do you mean that you want a doctest that checks that the new srg is available through the general constructor (like we did for designs)?

Nathann

comment:8 in reply to:  7 Changed 7 years ago by Vincent Delecroix

Replying to ncohen:

Do you mean that you want a doctest that checks that the new srg is available through the general constructor (like we did for designs)?

Yes, would be good to check that the construction is actually useful...

sage: graphs.strongly_regular_graph(my_new_parameters)
!Got one!

comment:9 Changed 7 years ago by git

Commit: 9fc78a492823ede303f45a4a6238d06a0c20f87e5a0384c60fb35730a87d96db3a983d0cf27610d6

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

5a0384ctrac #19224: Rework the doctests

comment:10 Changed 7 years ago by Dima Pasechnik

Status: needs_reviewneeds_work

you do not test anywhere that graphs you construct in this functions are SRGs with parameters you claim they will have. IMHO such tests must be present.

comment:11 Changed 7 years ago by Nathann Cohen

I do in the doctest that builds the SRG by calling graphs.strongly_regular_graph (remember that it checks its output). Before, I had the usual "build the graph + test" doctest, but Vincent wanted me to query the general constructor instead.

So well, this doctest actually builds the graph and tests it. Like the one that was there before it.

Nathann

comment:12 in reply to:  11 Changed 7 years ago by Dima Pasechnik

Replying to ncohen:

I do in the doctest that builds the SRG by calling graphs.strongly_regular_graph (remember that it checks its output). Before, I had the usual "build the graph + test" doctest, but Vincent wanted me to query the general constructor instead.

So well, this doctest actually builds the graph and tests it. Like the one that was there before it.

it does not strike me as a reliable way to test; if the implementation of graphs.strongly_regular_graph changes, you'd be SOL. And testing only one tuple of parameters looks a bit too weak, too...

comment:13 Changed 7 years ago by git

Commit: 5a0384c60fb35730a87d96db3a983d0cf27610d69433564213a8a699814106c375b4cc0a5ef22f71

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

9433564trac #19224: Additional doctests

comment:14 Changed 7 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:15 Changed 7 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

OK

comment:16 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_work

Reviewer name

comment:17 Changed 7 years ago by Nathann Cohen

Reviewers: Dima Pasechnik
Status: needs_workpositive_review

Dima. Please.. -_-

comment:18 Changed 7 years ago by Volker Braun

Branch: u/ncohen/192249433564213a8a699814106c375b4cc0a5ef22f71
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.