Opened 6 years ago
Closed 6 years ago
#19018 closed enhancement (fixed)
More SRGs using Regular Symmetric Hadamard matric with Constant Diagonal
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.9 |
Component: | graph theory | Keywords: | |
Cc: | dimpase, vdelecroix, knsam | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | 6ed716d (Commits, GitHub, GitLab) | Commit: | 6ed716d1fcda90aaf146229b96c389b70ca39576 |
Dependencies: | Stopgaps: |
Description
With this branch, we have some constructions of Regular Symmetric Hadamard matric with Constant Diagonal (RSHCD), as well as new SRG constructor (often picked from the RSHCD litterature).
Change History (58)
comment:1 Changed 6 years ago by
- Branch set to u/ncohen/19018
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit set to a414c366744bfd1bd56618994e6ab83dc4a8b277
comment:3 follow-up: ↓ 5 Changed 6 years ago by
+`\delta\in\{-1,+1\}` and row values all equal to `\delta \epsilon + \sqrt(n)`. For more information, .... + [BrouwerSpectra11]_.
you probably mean
+`\delta\in\{-1,+1\}` and row sums all equal to `\delta = \epsilon + \sqrt(n)`. For more information, .... + [BrouwerSpectra11]_.
Also, [BrouwerSpectra11] has two authors, it's not good to have only one name in the label. And the year is wrong. In #18972 I refer to this as [BH12]
.. [BH12] A. E. Brouwer, W. H. Haemers, + Spectra of Graphs, + Springer, 2012 + http://dx.doi.org/10.1007/978-1-4614-1939-6
Let's put it here in this way, [BH12], OK? Less hassle for me on #18972 then.
comment:4 Changed 6 years ago by
- Commit changed from a414c366744bfd1bd56618994e6ab83dc4a8b277 to 0e533bb0f56992ed9e79e808fb2efe83d6ac0ab5
comment:5 in reply to: ↑ 3 Changed 6 years ago by
you probably mean
No. I meant what is written. Look at the other reference (not Brouwer) that I cite after this paragraph. I added it there because it has been rather difficult to figure out what was meant by (n,epsilon)-RSHCD. That reference defines it clearly. By the way both \delta
and \epsilon
belong to -1,1
, so the equation you wrote is never satisfied.
Let's put it here in this way, [BH12], OK? Less hassle for me on #18972 then.
Fixed.
I also added a commit I had forgotten: otherwise the new constructions are not used in graphs.strongly_regular_graph
.
Nathann
comment:6 follow-up: ↓ 7 Changed 6 years ago by
- Status changed from needs_review to needs_work
doc does not build
comment:7 in reply to: ↑ 6 Changed 6 years ago by
doc does not build
Could you please double-check that the "failed build" is caused by this ticket?
See https://groups.google.com/forum/#!topic/sage-devel/tQFsu-LWto0
Nathann
comment:8 Changed 6 years ago by
Yes it is. Look just a little be before in the shortlog:
OSError: [combinat ] None:6: WARNING: citation not found: BH11
comment:9 Changed 6 years ago by
- Commit changed from 0e533bb0f56992ed9e79e808fb2efe83d6ac0ab5 to 6893ad1163b8c636fe4f54e3ff838ccad85f4d5b
Branch pushed to git repo; I updated commit sha1. New commits:
6893ad1 | trac #19018: Typo
|
comment:10 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:11 follow-up: ↓ 12 Changed 6 years ago by
you must also declare utf8 encoding in the file "src/sage/graphs/strongly_regular_db.pyx"
because of Jørgensen
comment:12 in reply to: ↑ 11 Changed 6 years ago by
you must also declare utf8 encoding in the file "src/sage/graphs/strongly_regular_db.pyx"
because of Jørgensen
I compile the code and the doc without a problem. Why is this required?
comment:13 follow-up: ↓ 14 Changed 6 years ago by
I am not so sure. I have spend some time to do a better "ascii_plugin". I made it so that it now complains only in presence of UTF8 characters in .py or .pyx files without utf8 declaration in the preamble. Which is the case here.
There is https://www.python.org/dev/peps/pep-0263/
But is seems to be useful only if you want unicode in the source. Maybe encoding declaration in presence of uft8 can be considered good practice anyway ?
comment:14 in reply to: ↑ 13 Changed 6 years ago by
Maybe encoding declaration in presence of uft8 can be considered good practice anyway ?
Maybe it can. I personally do not care much. If it is important to you, please add a commit.
Nathann
P.S.: detecting whether a file contains utf8 characters can be done automatically. So we should not have to add it manually. Maybe this is the reason why this is apparently managed automatically.
comment:15 follow-up: ↓ 16 Changed 6 years ago by
I do not like how (100, 44, 18, 20)-srg and (100, 45, 20, 20) are specified. Why don't you present them as Cayley graphs? Further, the paper you cite constructs several nonisomorphic examples of these graphs, and you don't say which ones you give.
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 18 Changed 6 years ago by
I do not like how (100, 44, 18, 20)-srg and (100, 45, 20, 20) are specified. Why don't you present them as Cayley graphs?
Because it takes several minutes to build it, and that you need to work around a bug I reported that people are still discussing (and I won't take part in it anymore)
https://groups.google.com/d/topic/sage-devel/6rXKkF87Gtc/discussion
Further, the paper you cite constructs several nonisomorphic examples of these graphs, and you don't say which ones you give.
I do not think that it matters. I just want one.
Nathann
comment:17 Changed 6 years ago by
By the way, Volker was suggesting that '==' in groups would not be group equality but representation equality. If you enjoy that kind of fun join them. But I don't have that kind of patience.
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 19 Changed 6 years ago by
Replying to ncohen:
I do not like how (100, 44, 18, 20)-srg and (100, 45, 20, 20) are specified. Why don't you present them as Cayley graphs?
Because it takes several minutes to build it,
huh? Here is an experiment with GAP, which is almost instant to run:
gap> g:=CyclicGroup(100); <pc group of size 100 with 4 generators> gap> l:=List([1..45], x->Random(g)); [ f2*f3, f2*f3^4*f4^2, f1*f3^4*f4^2, f1*f4^4, f2*f3^3*f4^4, f2*f3^3*f4^3, f2*f4^2, f4, f2*f3^3*f4^3, f1*f4, f1*f4^3, f2*f3^4, f1*f2*f4^4, f1*f3^4*f4^4, f3^3, f1*f3^3*f4^4, f1*f3^3*f4, f2*f3^4*f4^4, f1*f3^2, f3^3*f4^4, f2*f3^2*f4, f1*f3^2*f4^3, f2*f3^4, f2*f3^3*f4^2, f2*f3^3*f4, f1*f3^3*f4, f3*f4, f2*f3^3*f4^4, f3^2*f4, f1*f2*f3^4*f4^4, f3^4*f4^4, f1*f3^3*f4, f1*f3^4*f4^2, f1*f3^2*f4^2, f2*f3^4*f4^3, f1*f2*f3, f3, f1*f3^4, f2*f4^4, f1*f3^2, f2*f3^3, f3^2*f4, f3*f4^4, f2*f3^3*f4^2, f4^3 ] gap> LoadPackage("grape"); true gap> G:=CayleyGraph(g,l); rec( adjacencies := [ [ 2, 3, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 16, 20, 21, 22, 23, 25, 28, 32, 33, 34, 35, 37, 41, 43, 46, 47, 48, 50, 51, 52, 54, 55, 59, 60, 62, 63, 64, 67, 70, 71, 72, 74, 75, 76, 80, 83, 84, 86, 87, 90, 92, 93, 94, 95, 96, 98, 99, 100 ] ], group := <permutation group of size 100 with 4 generators>, isGraph := true, isSimple := true, names := [ <identity> of ..., f1, f2, f3, f4, f1*f2, f1*f3, f1*f4, f2*f3, f2*f4, f3^2, f3*f4, f4^2, f1*f2*f3, f1*f2*f4, f1*f3^2, f1*f3*f4, f1*f4^2, f2*f3^2, f2*f3*f4, f2*f4^2, f3^3, f3^2*f4, f3*f4^2, f4^3, f1*f2*f3^2, f1*f2*f3*f4, f1*f2*f4^2, f1*f3^3, f1*f3^2*f4, f1*f3*f4^2, f1*f4^3, f2*f3^3, f2*f3^2*f4, f2*f3*f4^2, f2*f4^3, f3^4, f3^3*f4, f3^2*f4^2, f3*f4^3, f4^4, f1*f2*f3^3, f1*f2*f3^2*f4, f1*f2*f3*f4^2, f1*f2*f4^3, f1*f3^4, f1*f3^3*f4, f1*f3^2*f4^2, f1*f3*f4^3, f1*f4^4, f2*f3^4, f2*f3^3*f4, f2*f3^2*f4^2, f2*f3*f4^3, f2*f4^4, f3^4*f4, f3^3*f4^2, f3^2*f4^3, f3*f4^4, f1*f2*f3^4, f1*f2*f3^3*f4, f1*f2*f3^2*f4^2, f1*f2*f3*f4^3, f1*f2*f4^4, f1*f3^4*f4, f1*f3^3*f4^2, f1*f3^2*f4^3, f1*f3*f4^4, f2*f3^4*f4, f2*f3^3*f4^2, f2*f3^2*f4^3, f2*f3*f4^4, f3^4*f4^2, f3^3*f4^3, f3^2*f4^4, f1*f2*f3^4*f4, f1*f2*f3^3*f4^2, f1*f2*f3^2*f4^3, f1*f2*f3*f4^4, f1*f3^4*f4^2, f1*f3^3*f4^3, f1*f3^2*f4^4, f2*f3^4*f4^2, f2*f3^3*f4^3, f2*f3^2*f4^4, f3^4*f4^3, f3^3*f4^4, f1*f2*f3^4*f4^2, f1*f2*f3^3*f4^3, f1*f2*f3^2*f4^4, f1*f3^4*f4^3, f1*f3^3*f4^4, f2*f3^4*f4^3, f2*f3^3*f4^4, f3^4*f4^4, f1*f2*f3^4*f4^3, f1*f2*f3^3*f4^4, f1*f3^4*f4^4, f2*f3^4*f4^4, f1*f2*f3^4*f4^4 ], order := 100, representatives := [ 1 ], schreierVector := [ -1, 1, 2, 3, 4, 2, 3, 4, 3, 4, 3, 4, 4, 3, 4, 3, 4, 4, 3, 4, 4, 3, 4, 4, 4, 3, 4, 4, 3, 4, 4, 4, 3, 4, 4, 4, 3, 4, 4, 4, 4, 3, 4, 4, 4, 3, 4, 4, 4, 4, 3, 4, 4, 4, 4, 4, 4, 4, 4, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4 ] )
in your case the group will be given by 3 permutation generators, but this will only make it even faster.
Further, the paper you cite constructs several nonisomorphic examples of these graphs, and you don't say which ones you give.
I do not think that it matters. I just want one.
How hard is to say that in a comment that you used pds such-and-such for group such-and-such?
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 21 Changed 6 years ago by
huh? Here is an experiment with GAP, which is almost instant to run:
Oh. Good news. Well, the best would be to rely on GAP for these computations then, for Sage does it *very* slowly.
in your case the group will be given by 3 permutation generators, but this will only make it even faster.
If you can make it work fast in Sage, I have no objection. Here is the code I used to generate it:
from sage.groups.finitely_presented import FinitelyPresentedGroup G = FreeGroup('x,y,z') x,y,z = G.gens() rels = (x**5,y**5,z**4,x*y*x**(-1)*y**(-1),z*x*z**(-1)*x**(-2),z*y*z**(-1)*y**(-2)) G = FinitelyPresentedGroup(G,rels) x,y,z = G.gens() H = G.as_permutation_group() L=["120","140","200","210","201","401","411","321","002","012","022","042","303","403","013","413","240","031","102","323","300","231","132","133","310","141","142","233","340","241","202","333","410","341","222","433","430","441","242","302","312","322","332","442","143"] L = map(lambda x:map(int,x),L) L = [G(x**xx*y**yy*z**zz) for xx,yy,zz in L] GG = Graph() for v in G: for u in L: uv = u*v GG.add_edge([v]+[x for x in G if x==uv]) print GG.size()
How hard is to say that in a comment that you used pds such-and-such for group such-and-such?
I just do not care at all. That's why I did not. Can you acknowledge that others can have a different view on this?
I will add a commit, just to end the discussion.
Nathann
comment:20 Changed 6 years ago by
- Commit changed from 6893ad1163b8c636fe4f54e3ff838ccad85f4d5b to d66f0b651e5fe52827aeb21b4c05e4c3cf5c7625
Branch pushed to git repo; I updated commit sha1. New commits:
d66f0b6 | trac #19018: Specify which graph is built
|
comment:21 in reply to: ↑ 19 ; follow-up: ↓ 22 Changed 6 years ago by
Replying to ncohen:
huh? Here is an experiment with GAP, which is almost instant to run:
Oh. Good news. Well, the best would be to rely on GAP for these computations then, for Sage does it *very* slowly.
in your case the group will be given by 3 permutation generators, but this will only make it even faster.
If you can make it work fast in Sage, I have no objection. Here is the code I used to generate it:
how about you add this code as an optional method to construct this graph (and analogous code for the other graph), and # long time tests showing that these are the graphs you claim they are.
How hard is to say that in a comment that you used pds such-and-such for group such-and-such?
I just do not care at all. That's why I did not. Can you acknowledge that others can have a different view on this?
I can contact an author of the paper in question I know, and ask his opinion. You will be in for a shock then ;-) To many people a reference of the form "this is one I fished out from [Blah]" is too vague, not only to me.
I will add a commit, just to end the discussion.
Could you change in [JK03]_.
to in Tab.8.1 from [JK03]_.
there?
Otherwise you force the person interested to browse through most of the paper, and it is 30+ pages long.
comment:22 in reply to: ↑ 21 Changed 6 years ago by
Hello,
how about you add this code as an optional method to construct this graph (and analogous code for the other graph), and # long time tests showing that these are the graphs you claim they are.
I do not want to, especially when the code is badly written in order to avoid a bug. If you care, you can add a commit. But really, it would be better to wait for the bug to be solved before writing that.
I can contact an author of the paper in question I know, and ask his opinion. You will be in for a shock then ;-)
It would change nothing even if the author has the same opinion. The status is simple: *I* do not care, which is why I did not do it. That ends your means to force me to do it. If you want to do it, there is no problem, as usual.
As for the authors, I sent them an email this morning to tell them what I did, and to thank them for the instructions.
Could you change
in [JK03]_.
toin Tab.8.1 from [JK03]_.
there? Otherwise you force the person interested to browse through most of the paper, and it is 30+ pages long.
Will do.
Nathann
comment:23 Changed 6 years ago by
- Commit changed from d66f0b651e5fe52827aeb21b4c05e4c3cf5c7625 to 1cae2dc761cd1e784f0a5a4239d90f1aa6ea48ce
Branch pushed to git repo; I updated commit sha1. New commits:
1cae2dc | trac #19028: Table 8.1
|
comment:24 follow-up: ↓ 25 Changed 6 years ago by
P.S.: If you add such tests, please mark them as "not tested", and not as "long time". I already mark as such the tests that go above 2seconds. When you have too many of them, running long tests on that file takes minutes off everybody's time. Perhaps we should have a --very-long
flag.... :-/
Nathann
comment:25 in reply to: ↑ 24 ; follow-ups: ↓ 26 ↓ 27 Changed 6 years ago by
Replying to ncohen:
P.S.: If you add such tests, please mark them as "not tested", and not as "long time". I already mark as such the tests that go above 2seconds. When you have too many of them, running long tests on that file takes minutes off everybody's time. Perhaps we should have a
--very-long
flag....:-/
well, it's your Sage code that does something very slow. Here is a slight modification that is very fast:
def g1(): from sage.groups.finitely_presented import FinitelyPresentedGroup G = FreeGroup('x,y,z') x,y,z = G.gens() rels = (x**5,y**5,z**4,x*y*x**(-1)*y**(-1),z*x*z**(-1)*x**(-2),z*y*z**(-1)*y**(-2)) G = FinitelyPresentedGroup(G,rels) x,y,z = G.gens() H = G.as_permutation_group() L=["120","140","200","210","201","401","411","321","002","012","022","042","303","403","013","413","240","031","102","323","300","231","132","133","310","141","142","233","340","241","202","333","410","341","222","433","430","441","242","302","312","322","332","442","143"] L = map(lambda x:map(int,x),L) x,y,z=(H.0,H.1,H.2) # from now on we work in H L = [H(x**xx*y**yy*z**zz) for xx,yy,zz in L] GG = Graph() for v in H: GG.add_edges(map(lambda u: (v,u*v), L)) # and we add edges in bunches return GG
gives
sage: timeit('G=g1()') 25 loops, best of 3: 13.7 ms per loop sage: G.is_strongly_regular(parameters=True) (100, 45, 20, 20)
Would you like to change the code to use this way to build these graphs, or prefer me doing it? In the latter case I'd appreciate having strings in L for the other graph...
comment:26 in reply to: ↑ 25 Changed 6 years ago by
Replying to dimpase:
Here is a slight modification that is very fast:
def g1(): from sage.groups.finitely_presented import FinitelyPresentedGroup G = FreeGroup('x,y,z') x,y,z = G.gens() rels = (x**5,y**5,z**4,x*y*x**(-1)*y**(-1),z*x*z**(-1)*x**(-2),z*y*z**(-1)*y**(-2)) G = FinitelyPresentedGroup(G,rels) x,y,z = G.gens() H = G.as_permutation_group() L=["120","140","200","210","201","401","411","321","002","012","022","042","303","403","013","413","240","031","102","323","300","231","132","133","310","141","142","233","340","241","202","333","410","341","222","433","430","441","242","302","312","322","332","442","143"] L = map(lambda x:map(int,x),L) x,y,z=(H.0,H.1,H.2) # from now on we work in H L = [H(x**xx*y**yy*z**zz) for xx,yy,zz in L] GG = Graph() for v in H: GG.add_edges(map(lambda u: (v,u*v), L)) # and we add edges in bunches return GG
in fact the lines
GG = Graph() for v in H: GG.add_edges(map(lambda u: (v,u*v), L)) # and we add edges in bunches
can be replaced by
GG = Graph(H.cayley_graph(generators=L, simple=True))
comment:27 in reply to: ↑ 25 ; follow-up: ↓ 30 Changed 6 years ago by
well, it's your Sage code that does something very slow.
You really have your way to phrase things, Dima... Clearly there is no slowness in Sage, and I was doing it very badly.
At some point I wanted to try the permutation group version, but I did not know how to get the generators back in this version of the group. Despite the fact that it indeed works, how can you be sure that you will get the same generators, in the same order?
Would you like to change the code to use this way to build these graphs, or prefer me doing it?
You want that feature, and you apparently know more than I on the matter.
In the latter case I'd appreciate having strings in L for the other graph...
I don't have it anymore. I overwrote it with this second list.
Nathann
comment:28 Changed 6 years ago by
- Commit changed from 1cae2dc761cd1e784f0a5a4239d90f1aa6ea48ce to 704512495585867e857be7546e45cfa1d511a387
Branch pushed to git repo; I updated commit sha1. New commits:
7045124 | trac #19018: Merged with 6.9.beta2
|
comment:29 Changed 6 years ago by
(there was a conflict with the latest beta)
comment:30 in reply to: ↑ 27 Changed 6 years ago by
Replying to ncohen:
At some point I wanted to try the permutation group version, but I did not know how to get the generators back in this version of the group. Despite the fact that it indeed works, how can you be sure that you will get the same generators, in the same order?
Signature: G.as_permutation_group(limit=4096000) Docstring: Return an isomorphic permutation group. The generators of the resulting group correspond to the images by the isomorphism of the generators of the given group. ...
is not clear enough?
comment:31 Changed 6 years ago by
It is.
Nathann
comment:32 Changed 6 years ago by
Oh, I can use _helper_payley_matrix
you have written :-)
comment:33 follow-up: ↓ 34 Changed 6 years ago by
What for ? O_o
comment:34 in reply to: ↑ 33 Changed 6 years ago by
- Branch changed from u/ncohen/19018 to public/19018
- Commit changed from 704512495585867e857be7546e45cfa1d511a387 to 80846e1bd3695f0fd512c6257ac8961456939243
comment:35 Changed 6 years ago by
- Reviewers set to Dima Pasechnik
if you are happy with my added commit, feel free to set it to positive review.
comment:36 follow-up: ↓ 37 Changed 6 years ago by
Come on Dima... This function is only a 'cdef' function because you do not want to write doctests... It is not so bad to copy/paste code (twice) here.
Nathann
comment:37 in reply to: ↑ 36 ; follow-up: ↓ 38 Changed 6 years ago by
Replying to ncohen:
Come on Dima... This function is only a 'cdef' function because you do not want to write doctests... It is not so bad to copy/paste code (twice) here.
No, no, no, creation of the same group twice is a deadly sin in my Book!
(besides, SRG_100_44_18_20
and SRG_100_45_20_20
doctest it good enough)
And it's cdef
because it is a helper function.
comment:38 in reply to: ↑ 37 Changed 6 years ago by
No, no, no, creation of the same group twice is a deadly sin in my Book! (besides,
SRG_100_44_18_20
andSRG_100_45_20_20
doctest it good enough)
This is ridiculous. But if you insist:
1) Make it a Python function
2) Doctest it properly
3) Rename it with a _
before its name, so that it is clearly a private function
I also find all this to be a waste of time. This is why I believe that copy/paste is better.
Nathann
comment:39 follow-up: ↓ 40 Changed 6 years ago by
In "my" book, functions must be doctested unless you can't. Here, you can.
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 42 Changed 6 years ago by
Replying to ncohen:
In "my" book, functions must be doctested unless you can't. Here, you can.,
you have a number of such cdef'd functions in this file, e.g. eigenvalues
. It is clear it can be doctested too...
In H_3_cayley_graph()
case, it's clear it's basically a macro used in two different places, for this fine language forgot to have macros implemented... Would you like me to convert it into a decorator?!
comment:41 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:42 in reply to: ↑ 40 ; follow-up: ↓ 43 Changed 6 years ago by
- Status changed from positive_review to needs_work
you have a number of such cdef'd functions in this file, e.g.
eigenvalues
. It is clear it can be doctested too...
This function is a cdef function because it needs to be fast. So it can't.
In
H_3_cayley_graph()
case, it's clear it's basically a macro used in two different places, for this fine language forgot to have macros implemented... Would you like me to convert it into a decorator?!
I want you to do the job properly: either you copy paste it, or you document what you write. You don't turn a function into a 'cdef' function just because you don't want to write documentation.
Besides, you don't give a positive review to your own changes. Period.
Nathann
comment:43 in reply to: ↑ 42 ; follow-up: ↓ 44 Changed 6 years ago by
Replying to ncohen:
you have a number of such cdef'd functions in this file, e.g.
eigenvalues
. It is clear it can be doctested too...This function is a cdef function because it needs to be fast. So it can't.
And we want H_3_cayley_graph()
to be slow, and turn it into a pure Python function due to this? Are you kidding me?
In
H_3_cayley_graph()
case, it's clear it's basically a macro used in two different places, for this fine language forgot to have macros implemented... Would you like me to convert it into a decorator?!I want you to do the job properly: either you copy paste it, or you document what you write. You don't turn a function into a 'cdef' function just because you don't want to write documentation.
Copy-pasting 10 lines of code is clearly a very bad idea. I do not understand why you even propose this.
comment:44 in reply to: ↑ 43 ; follow-up: ↓ 46 Changed 6 years ago by
And we want
H_3_cayley_graph()
to be slow, and turn it into a pure Python function due to this? Are you kidding me?
Make me laugh, tell me that you declared a function that builds a graph from a group into a C function because of a speed issue.
Copy-pasting 10 lines of code is clearly a very bad idea. I do not understand why you even propose this.
Because it does not require a new function, documentation, and doctests.
Nathann
comment:45 Changed 6 years ago by
Oh. Perhaps you missed something: changing the def/cdef only means that the *function call* occurs in C. What is being done inside is the same, and does not depend on how you declare the function.
comment:46 in reply to: ↑ 44 ; follow-up: ↓ 47 Changed 6 years ago by
Replying to ncohen:
Copy-pasting 10 lines of code is clearly a very bad idea. I do not understand why you even propose this.
Because it does not require a new function, documentation, and doctests.
And it makes the code stink. Why do you ask me to make code stink?
comment:47 in reply to: ↑ 46 ; follow-up: ↓ 48 Changed 6 years ago by
And it makes the code stink. Why do you ask me to make code stink?
Grow up. No God above forbids you to copy paste 10 lines of code. If you don't want to do that, then write the function according to Sage's standard. If you find this painful, then think again of the advantages of copy paste.
Nathann
comment:48 in reply to: ↑ 47 Changed 6 years ago by
Replying to ncohen:
And it makes the code stink. Why do you ask me to make code stink?
Grow up. No God above forbids you to copy paste 10 lines of code. If you don't want to do that, then write the function according to Sage's standard. If you find this painful, then think again of the advantages of copy paste.
Gods forbid us to waste time and space. This discussion is a waste of time, and copy/pasting code is waste of space, and of time, for someone who will have to read these chunks of brainlessly duplicated code. And it is clear that copy/pasted code has no place in a quality codebase.
Nathann
comment:49 follow-up: ↓ 50 Changed 6 years ago by
Write this doctest, and let's move on.
comment:50 in reply to: ↑ 49 ; follow-up: ↓ 51 Changed 6 years ago by
Replying to ncohen:
Write this doctest, and let's move on.
One cannot directly doctest cdef
'd functions, right?
comment:51 in reply to: ↑ 50 Changed 6 years ago by
One cannot directly doctest
cdef
'd functions, right?
Dima, this function has no reason to be a cdef. So write its docstring correctly or remove your commit: my branch is correct and respects the standards.
Nathann
comment:52 follow-up: ↓ 53 Changed 6 years ago by
Just in case it might convince you that copy/paste is better: note that the paper indicates that H3 is the 11th group in GAP's catalogue of groups of order 100. So you don't even need to build it by yourself.
Nathann
comment:53 in reply to: ↑ 52 Changed 6 years ago by
Replying to ncohen:
Just in case it might convince you that copy/paste is better: note that the paper indicates that H3 is the 11th group in GAP's catalogue of groups of order 100. So you don't even need to build it by yourself.
I know, but this means that one would need database_gap
optional package installed. If this is OK with you then I can change the code to use instead, and this would allow to get rid of the code of H_3_cayley_graph
.
comment:54 Changed 6 years ago by
HMmmm.... No sorry, I'd like this function to be available in pure Sage (without optional packages). If you don't mind, then could you make both available? The 'graph6' version and the group one? This way it can be built directly in Sage, but we can still check that it is valid (and optional tests are automatically run now).
Nathann
comment:55 Changed 6 years ago by
- Commit changed from 80846e1bd3695f0fd512c6257ac8961456939243 to 6ed716d1fcda90aaf146229b96c389b70ca39576
Branch pushed to git repo; I updated commit sha1. New commits:
6ed716d | added a doctest
|
comment:56 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:57 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:58 Changed 6 years ago by
- Branch changed from public/19018 to 6ed716d1fcda90aaf146229b96c389b70ca39576
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #19018: More SRGs using Regular Symmetric Hadamard matric with Constant Diagonal