Opened 5 years ago
Closed 5 years ago
#19224 closed enhancement (fixed)
swtich OA(k,n)+* 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:  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 5 years ago by
 Branch set to u/ncohen/19224
 Commit set to 92a2a6fb4bce16667412a5d206133d6128b25c00
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 5 years ago by
 Status changed from needs_review to needs_work
Hello,
 You wrote
from math import sqrt cdef int n = int(sqrt(n_2_p_11))
Sage integers do have a methodsqrtrem
that gives you the square root and the restsage: 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 functionsfrom libc.math cimport sqrt, floor
 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 functiondef switch_OA_srg(c,n): ... OA = map(tuple, orthogonal_array(c+1,n,resolvable=True)) ...
 Use better spacings:
int v,int k,int l
>int v, int k, int l
.
 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 codesage: 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
 Could you add an example of a SRG that was not available before?
Vincent
comment:3 Changed 5 years ago by
 Commit changed from 92a2a6fb4bce16667412a5d206133d6128b25c00 to 9fc78a492823ede303f45a4a6238d06a0c20f87e
comment:4 in reply to: ↑ 2 ; followup: ↓ 6 Changed 5 years ago by
Hello,
 You wrote
from math import sqrt cdef int n = int(sqrt(n_2_p_11))
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.
 Why are you testing withtout the option
resolvable=True
That's a mistake indeed.
 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.
 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.
 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 5 years ago by
 Status changed from needs_work to needs_review
comment:6 in reply to: ↑ 4 Changed 5 years ago by
Replying to ncohen:
 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 followup: ↓ 8 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Commit changed from 9fc78a492823ede303f45a4a6238d06a0c20f87e to 5a0384c60fb35730a87d96db3a983d0cf27610d6
Branch pushed to git repo; I updated commit sha1. New commits:
5a0384c  trac #19224: Rework the doctests

comment:10 Changed 5 years ago by
 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 followup: ↓ 12 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Commit changed from 5a0384c60fb35730a87d96db3a983d0cf27610d6 to 9433564213a8a699814106c375b4cc0a5ef22f71
Branch pushed to git repo; I updated commit sha1. New commits:
9433564  trac #19224: Additional doctests

comment:14 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 5 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_work to positive_review
Dima. Please.. _
comment:18 Changed 5 years ago by
 Branch changed from u/ncohen/19224 to 9433564213a8a699814106c375b4cc0a5ef22f71
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #19224: swtich OA(k,n)+* strongly regular graphs