Opened 3 years ago

Closed 3 years ago

#19224 closed enhancement (fixed)

swtich OA(k,n)+* 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: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 9433564 (Commits) Commit: 9433564213a8a699814106c375b4cc0a5ef22f71
Dependencies: Stopgaps:

Description

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

Nathann

Change History (18)

comment:1 Changed 3 years ago by ncohen

  • Branch set to u/ncohen/19224
  • Commit set to 92a2a6fb4bce16667412a5d206133d6128b25c00
  • Status changed from new to needs_review

New commits:

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

comment:2 follow-up: Changed 3 years ago by vdelecroix

  • Status changed from needs_review to needs_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 3 years ago by git

  • Commit changed from 92a2a6fb4bce16667412a5d206133d6128b25c00 to 9fc78a492823ede303f45a4a6238d06a0c20f87e

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 ; follow-up: Changed 3 years ago by ncohen

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

  • Status changed from needs_work to needs_review

comment:6 in reply to: ↑ 4 Changed 3 years ago by vdelecroix

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 follow-up: Changed 3 years ago by 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)?

Nathann

comment:8 in reply to: ↑ 7 Changed 3 years ago by vdelecroix

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

  • Commit changed from 9fc78a492823ede303f45a4a6238d06a0c20f87e to 5a0384c60fb35730a87d96db3a983d0cf27610d6

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

5a0384ctrac #19224: Rework the doctests

comment:10 Changed 3 years ago by dimpase

  • Status changed from needs_review to needs_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 follow-up: Changed 3 years ago by 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.

Nathann

comment:12 in reply to: ↑ 11 Changed 3 years ago by dimpase

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

  • Commit changed from 5a0384c60fb35730a87d96db3a983d0cf27610d6 to 9433564213a8a699814106c375b4cc0a5ef22f71

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

9433564trac #19224: Additional doctests

comment:14 Changed 3 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:15 Changed 3 years ago by dimpase

  • Status changed from needs_review to positive_review

OK

comment:16 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:17 Changed 3 years ago by ncohen

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_work to positive_review

Dima. Please.. -_-

comment:18 Changed 3 years ago by vbraun

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