sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

skew-Hadamard matrices and related srg's #19418

Closed dimpase closed 8 years ago

dimpase commented 8 years ago

implementing few basic constructions of skew-Hadamard matrices and 3 families of assoc. srg's.

Depends on #19309

CC: @nathanncohen

Component: combinatorics

Author: Dima Pasechnik

Branch/Commit: c50c78d

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/19418

dimpase commented 8 years ago

Attachment: pa.py.gz

a draft implementation of the corresponding SRGs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 347b37e to 3453e16

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

3453e16merged...
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

bf8dcc1added Goethals-Seidel construction; this gives orders 36 and 52
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 3453e16 to bf8dcc1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9e0ff0fadded Goethals-Seidel construction; this gives orders 36, 52, and 92
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from bf8dcc1 to 9e0ff0f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

f4e3a06graph constructions added (prelim version, refs etc missing)
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 9e0ff0f to f4e3a06

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4aa73f7graph constructions added, with skew-normalizing Hadamard mats
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from f4e3a06 to 4aa73f7

dimpase commented 8 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+implementing few basic constructions of skew-Hadamard matrices and 3 families of assoc. srg's.
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:8

Hello Dima,

I just merged #19309 with the latest beta. Could you do rebase this branch on top of it (not merge) so that your commits appear on top of that other branch and are easier to review ?

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:9

(right now the branch consists of 13 commits, 10 of which are merges with tickets that belong to the latest release)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:10

I just did it: the rebased branch corresponding to this ticket can be found at public/19418.

Nathann

dimpase commented 8 years ago

Changed commit from 4aa73f7 to c5a7eee

dimpase commented 8 years ago

Changed branch from u/dimpase/skewhad to public/19418

dimpase commented 8 years ago

New commits:

8607575trac #19309: Polhill strongly regular graphs
2cd7910trac #19309: Note for later
7bbefd1trac #19309: Merged with 6.9.rc1
acba28btrac #19309: Broken doctest
732f243trac #19309: Merged with 6.10.beta0
e65da7etrac #19309: Merged with 6.10.beta1
c5a7eeetrac #19418: skew-Hadamard matrices and related srg's
dimpase commented 8 years ago
comment:13

I assume you meant switching to this branch by 'work' :-) By the way, I've left a function _L_g_n_params in the source (it's actually not used). If you think it should be removed, I will do so.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:14

Helloooooo Dima,

First-pass review:

+    if M is None: # try Williamson-Goethals-Seidel construction
+        if _GS_skew_hadamard(n, existence=True):

becomes

+    if M is None and_GS_skew_hadamard(n, existence=True):

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago

Reviewer: Nathann Cohen

dimpase commented 8 years ago
comment:15

Replying to @nathanncohen:

First-pass review:

thanks, and apologies for sloppy code (back to Oxford with huge jetlag)

  • Why don't you call normalise_hadamard instead of adding a keyword inside of hadamard_matrix_paleyI (without changing hadamard_matrix_paleyII)? Those two functions are not directly available to the users, who are meant to call hadamard_matrix directly.

normalize_hadamard does not do what I need; I need a different type of normalization, in fact, where the matrix has the 1st row consisting of 1s, and still H+H.T==2*I, i.e. the 1st column of H is all -1 (with exception of the top left entry)

Should I perhaps introduce skew_normalize_hadamard, that can only be applied to skew Hadamard matrices? (I'd have uses for it in graph constructions, too)

  • is_hadamard_martrix -- could you move 'skew=False' to before 'verbose=False' ? Usually the verbosity/check flags are the last ones to appear as they do not change the behaviour of the functon.

right. I forgot that keyword args can be positional, too...

  • Call is_skew_symmetric instead of doing it yourself. In theory you shouldn't have to create two copies of the matrix in memory in order to check that.

  • zero_position=1 -- you have to write documentation for private functions too. Does [the paraeter that you add] appear in the lemma cited in the docstring?

  • zero_position=1 -- if you meant True, write True.

  • _circulant_matrix -- if it does not exist yet, this constructor should be added in the matrix/ code and be available through matrix.<tab>

OK

  • mod(j-i,n) is (j-i)%n. One day you will have to accept that you write Python code.

  • Instead of a _GS_skew_hadamard function that encodes 4 matrices, why don't you reserve cases if n == 36' (and others) in the main function skew_hadamard_matrix`? That would also solve the problem that this function has no documentation whatsoever.

I'd rather add documentation, thanks for reminding. I find skew_hadamard_matrix already hard to read, and it would positively turn ugly if I start adding that explicit data for matrices there.

+    if M is None: # try Williamson-Goethals-Seidel construction
+        if _GS_skew_hadamard(n, existence=True):

becomes

+    if M is None and_GS_skew_hadamard(n, existence=True):

oops - there is an extra if there that should go...

  • switch_skewhad_pow2 -- please respect the current standard for the names in graphs.<tab>. Upper case, no underscore, ends with Graph. Actually, it could be named 'SwitchSkewhadGraphand only mention thepow2` part in the docstring.

  • graphs.Pseudo_L_2n_4n_m_1? Seriously? If you cannot give it a clear and meaningful name easily, then take it as a sign that this function should not be exposed to the users directly.

How about we have a PseudoLatinSquaresGraph(m,n) or PseudoOrthogonalArrayBlockGraph(m,n) ? (I'd prefer the former name; or maybe we can have both?)

It would include Pseudo_L_2n_4n_m_1 and OrthogonalArrayBlockGraph as subcases. (Perhaps there are more subcases, in fact, but we don't have to worry about it now).

  • _L_g_n_params -- yes, please remove this function if you do not use it.

sure.

dimpase commented 8 years ago

Dependencies: #19309

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:17

Helloooooooooooooooooooo,

(back to Oxford with huge jetlag)

Wow. Common timezone again. Cool ;-)

normalize_hadamard does not do what I need; I need a different type of normalization, in fact, where the matrix has the 1st row consisting of 1s, and still H+H.T==2*I, i.e. the 1st column of H is all -1 (with exception of the top left entry)

Should I perhaps introduce skew_normalize_hadamard, that can only be applied to skew Hadamard matrices? (I'd have uses for it in graph constructions, too)

Sounds right. Then you can have a proper documentation and stuff. If you plan to use it elsewhere that's even better.

I'd rather add documentation, thanks for reminding. I find skew_hadamard_matrix already hard to read, and it would positively turn ugly if I start adding that explicit data for matrices there.

Okayokay.

How about we have a PseudoLatinSquaresGraph(m,n) or PseudoOrthogonalArrayBlockGraph(m,n) ? (I'd prefer the former name; or maybe we can have both?)

+1 to the former. With a description of what it is in its doc, if possible ^^;

Nathann

dimpase commented 8 years ago
comment:18

Replying to @nathanncohen:

(back to Oxford with huge jetlag)

Wow. Common timezone again. Cool ;-)

an hour difference, still :-)

normalize_hadamard does not do what I need; I need a different type of normalization, in fact, where the matrix has the 1st row consisting of 1s, and still H+H.T==2*I, i.e. the 1st column of H is all -1 (with exception of the top left entry)

Should I perhaps introduce skew_normalize_hadamard, that can only be applied to skew Hadamard matrices? (I'd have uses for it in graph constructions, too)

Sounds right. Then you can have a proper documentation and stuff. If you plan to use it elsewhere that's even better.

oops, this won't fly. The problem is that once a skew-Hadamard matrix was de-skewed (e.g. by the usual normalization procedure, producing the 1st row and column of 1s), there is no general way known to make it skew again. (and in fact not every Hadamard matrix can be made skew, AFAIK).

That is, I have to disable normalization right at the point it is done in hadamard_matrix_paleyI. (writing an ad hoc code to undo this specific normalization seems silly).

dimpase commented 8 years ago
comment:19

Replying to @nathanncohen:

Helloooooo Dima,

First-pass review:

  • Why don't you call normalise_hadamard instead of adding a keyword inside of hadamard_matrix_paleyI (without changing hadamard_matrix_paleyII)? Those two functions are not directly available to the users, who are meant to call hadamard_matrix directly.

  • is_hadamard_martrix -- could you move 'skew=False' to before 'verbose=False' ? Usually the verbosity/check flags are the last ones to appear as they do not change the behaviour of the functon.

  • Call is_skew_symmetric instead of doing it yourself. In theory you shouldn't have to create two copies of the matrix in memory in order to check that.

this won't fly either, as a skew Hadamard matrix H is not skew-symmetric. What is skew-symmetric is the matrix H-I, so it has to be created anyway

(by the way, this explains why making a Hadamard matrix skew is not too obvious; indeed, the trick mentioned in the docs of is_skew_symmetrizable would only work if H had constant diagonal.)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:20

Yo,

oops, this won't fly. The problem is that once a skew-Hadamard matrix was de-skewed (e.g. by the usual normalization procedure, producing the 1st row and column of 1s), there is no general way known to make it skew again. (and in fact not every Hadamard matrix can be made skew, AFAIK).

That is, I have to disable normalization right at the point it is done in hadamard_matrix_paleyI. (writing an ad hoc code to undo this specific normalization seems silly).

I cannot say that I understand what you have in mind, but as long as what happens is made clear in the documentation I guess that it is okay.

this won't fly either, as a skew Hadamard matrix H is not skew-symmetric. What is skew-symmetric is the matrix H-I, so it has to be created anyway

Then create H-I, but don't create -S nor S.T.

Nathann

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

0a5e36fadding missing docs and tests, part I
c284edamoved circulant to matrix.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from c5a7eee to c284eda

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:22

a couple of remarks about the last commit:

dimpase commented 8 years ago
comment:23

Replying to @nathanncohen:

a couple of remarks about the last commit:

  • except without the name of a specific exception makes Jeroen scream.

OK, sure, I will make it catch NoneType

  • It is a weird that the value of 'sparse' is ignored when 'v.is_sparse' says otherwise. The expected behaviour would be sparse=None by default (auto detect) and use the user-provided value otherwise.

I want to implement something like this:

    def f(vector v):
         stuff...

    def f(list v, sparse=False):
         stuff...

and this is what my code does.

The user-provided value of sparse for vector v is present in v already. If the user wants to make it explicit, he can call the function with the parameter vector(v, sparse=whatever) instead of just v

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

6f54887function renaming, documenting, and doctesting, and removing extra if
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from c284eda to 6f54887

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 6f54887 to 6b80f00

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

6b80f00added AttributeError to check for in except:
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

135c0eafor a skew Hadamard matrix, check that the diagonal entries are all 1
c04916breturn skew-normalized Hadamard mats by default
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 6b80f00 to c04916b

dimpase commented 8 years ago

Author: Dima Pasechnik

dimpase commented 8 years ago
comment:27

looks like I have addressed all the points raised; (I didn't mention in commit messages that the code now does checking of skew-Hadamardness inplace). ready for another round, hopefully...


New commits:

135c0eafor a skew Hadamard matrix, check that the diagonal entries are all 1
c04916breturn skew-normalized Hadamard mats by default
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from c04916b to 535e109

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

535e109putting reference in the right file
dimpase commented 8 years ago
comment:29

now make doc-clean && make passes.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 535e109 to b3fb30b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

b3fb30bMerge branch 'develop' of git://trac.sagemath.org/sage into skewhad
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

01763fdremove #random from paleyI/II and fix the test
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from b3fb30b to 01763fd

dimpase commented 8 years ago
comment:32

Replying to @sagetrac-git:

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

01763fdremove #random from paleyI/II and fix the test

I guess these # random were leftovers from old code. Let's get rid of them.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:33

I want to implement something like this:

    def f(vector v):
         stuff...

    def f(list v, sparse=False):
         stuff...

and this is what my code does.

The user-provided value of sparse for vector v is present in v already. If the user wants to make it explicit, he can call the function with the parameter vector(v, sparse=whatever) instead of just v

Usually the explicit value overrides the implicit value:

sage: Graph(graphs.PetersenGraph())._backend
<type 'sage.graphs.base.sparse_graph.SparseGraphBackend'>
sage: Graph(graphs.PetersenGraph(),sparse=False)._backend
<type 'sage.graphs.base.dense_graph.DenseGraphBackend'>

It is not the case in your code.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:34

could be something like that

def thing(v,sparse=None):
    if sparse is None:
        try:
            sparse = v.is_sparse()
        except AttributeError:
            sparse = False
dimpase commented 8 years ago
comment:35

Replying to @nathanncohen:

could be something like that

def thing(v,sparse=None):
    if sparse is None:
        try:
            sparse = v.is_sparse()
        except AttributeError:
            sparse = False

I'll be happy to fix it as you prefer. (You could also add a commit, of course :))