Opened 7 years ago

Closed 7 years ago

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

Reported by: Owned by: Nathann Cohen major sage-6.9 graph theory Dima Pasechnik Nathann Cohen Dima Pasechnik N/A 9433564 9433564213a8a699814106c375b4cc0a5ef22f71

Description

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

Nathann

comment:1 Changed 7 years ago by Nathann Cohen

Branch: → u/ncohen/19224 → 92a2a6fb4bce16667412a5d206133d6128b25c00 new → needs_review

New commits:

 ​92a2a6f `trac #19224: swtich OA(k,n)+* strongly regular graphs`

comment:2 follow-up:  4 Changed 7 years ago by Vincent Delecroix

Status: needs_review → 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 7 years ago by git

Commit: 92a2a6fb4bce16667412a5d206133d6128b25c00 → 9fc78a492823ede303f45a4a6238d06a0c20f87e

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

 ​580e422 `trac #19224: Merged with 6.9.beta7` ​9fc78a4 `trac #19224: Review`

comment:4 in reply to:  2 ; follow-up:  6 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?

Nathann

comment:5 Changed 7 years ago by Nathann Cohen

Status: needs_work → needs_review

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

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

I meant in the function `graphs.strongly_regular_graphs`.

Vincent

comment:7 follow-up:  8 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

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: 9fc78a492823ede303f45a4a6238d06a0c20f87e → 5a0384c60fb35730a87d96db3a983d0cf27610d6

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

 ​5a0384c `trac #19224: Rework the doctests`

comment:10 Changed 7 years ago by Dima Pasechnik

Status: needs_review → 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:  12 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

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: 5a0384c60fb35730a87d96db3a983d0cf27610d6 → 9433564213a8a699814106c375b4cc0a5ef22f71

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

 ​9433564 `trac #19224: Additional doctests`

comment:14 Changed 7 years ago by Nathann Cohen

Status: needs_work → needs_review

comment:15 Changed 7 years ago by Dima Pasechnik

Status: needs_review → positive_review

OK

comment:16 Changed 7 years ago by Volker Braun

Status: positive_review → needs_work

Reviewer name

comment:17 Changed 7 years ago by Nathann Cohen

Reviewers: → Dima Pasechnik needs_work → positive_review

Dima. Please.. `-_-`

comment:18 Changed 7 years ago by Volker Braun

Branch: u/ncohen/19224 → 9433564213a8a699814106c375b4cc0a5ef22f71 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.