Opened 5 years ago

Closed 5 years ago

Very careless typoes in strongly_regular_db

Reported by: Owned by: ncohen major sage-6.9 graph theory dimpase Nathann Cohen Dima Pasechnik N/A cb0b3fb (Commits) #19018

Description

This branch fixes very bad typoes in that file, that made several constructions useless or broken. Mostly missing characters (emacs macros...)

Nathann

comment:1 Changed 5 years ago by ncohen

• Branch set to u/ncohen/19019
• Status changed from new to needs_review

comment:2 Changed 5 years ago by git

• Commit set to 05b2fa885325eab312bd8380f22fe8690b14afae

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

 ​a414c36 trac #19018: More SRGs using Regular Symmetric Hadamard matric with Constant Diagonal ​05b2fa8 trac #19019: Very careless typoes in strongly_regular_db

comment:3 follow-up: ↓ 5 Changed 5 years ago by jmantysalo

Not relating to just this patch, but why functions like SRG_100_44_18_20(), SRG_100_45_20_20() etc. instead of something like SRG_from_db(100, 44, 18, 20)? There might be some rationale explained in some other ticket.

comment:4 Changed 5 years ago by git

• Commit changed from 05b2fa885325eab312bd8380f22fe8690b14afae to 4abbce4ea5a32d1b82eae7de85fa3dc5f695e5da

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

 ​c430b4b trac #19018: Link the new constructions with graphs.strongly_regular_graph ​0e533bb trac #19018: Bibliography ​4abbce4 trac #19019: Very careless typoes in strongly_regular_db

comment:5 in reply to: ↑ 3 Changed 5 years ago by ncohen

Not relating to just this patch, but why functions like SRG_100_44_18_20(), SRG_100_45_20_20() etc. instead of something like SRG_from_db(100, 44, 18, 20)? There might be some rationale explained in some other ticket.

I do not understand what you mean, and I do not want the conversation to happen on this unrelated ticket. Please send me an email or write to sage-devel.

Last edited 5 years ago by ncohen (previous) (diff)

comment:6 Changed 5 years ago by git

• Commit changed from 4abbce4ea5a32d1b82eae7de85fa3dc5f695e5da to c5e24be70ec523d6c91af7c7a0664cce9459e2cf

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

 ​c5e24be trac #19019: Missing space

comment:7 Changed 5 years ago by git

• Commit changed from c5e24be70ec523d6c91af7c7a0664cce9459e2cf to 3f76966eaf187f22485471c04f6227f22a024efe

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

 ​6893ad1 trac #19018: Typo ​e18bee7 trac #19019: Very careless typoes in strongly_regular_db ​3f76966 trac #19019: Missing space

comment:8 Changed 5 years ago by git

• Commit changed from 3f76966eaf187f22485471c04f6227f22a024efe to 19b40d996b2770b58bfefbea5bb8d3c0bded6047

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

 ​d66f0b6 trac #19018: Specify which graph is built ​1cae2dc trac #19028: Table 8.1 ​7045124 trac #19018: Merged with 6.9.beta2 ​99fa6ce trac #19019: Very careless typoes in strongly_regular_db ​cb071e2 trac #19019: Missing space ​19b40d9 trac #19019: Sort the list of SRGs

comment:9 Changed 5 years ago by dimpase

• Branch changed from u/ncohen/19019 to public/19019
• Commit changed from 19b40d996b2770b58bfefbea5bb8d3c0bded6047 to 778556f1afddb74933575180e36018f0608fba2a
• Status changed from needs_review to positive_review

rebased over the latest developments on #19018. LGTM

comment:10 Changed 5 years ago by jmantysalo

Volker will reject this if you don't add your name to Reviewers-field.

comment:11 follow-up: ↓ 12 Changed 5 years ago by git

• Commit changed from 778556f1afddb74933575180e36018f0608fba2a to 323416f9a5a46f1cc6765ef54906938353eee856
• Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

 ​323416f added a doctest

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

• Status changed from needs_review to positive_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

 ​323416f added a doctest

rebased over the latest #19018 branch

comment:13 Changed 5 years ago by ncohen

What you did is not a rebase, nor a merge. You apparently cherry-picked your commit on top of that branch (note that its hash changed). As a result your commit appears twice in the history.

Please remove this latest commit: it is not because the branch are not exactly the same that they do not merge trivially. I don't think that your last commit from #19018 caused the slightest problem with this branch.

Nathann

comment:14 Changed 5 years ago by vbraun

• Status changed from positive_review to needs_work

Reviewer name missing

comment:15 Changed 5 years ago by dimpase

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

I cannot find the extra commit in question.

comment:16 Changed 5 years ago by ncohen

It is the last commit of this branch.

comment:17 Changed 5 years ago by dimpase

In the local branch I do not have 2 copies of this commit. Naturally, if I were to remove it and push the result then it would not be there at all.

comment:18 Changed 5 years ago by ncohen

In the local branch you have this commit. In the branch of #19018 you have another copy of it. Because you cherry-picked it, when the two branches will be merged you will have two copies of it.

This commit belongs to the other branch, not to that one. If you remove it from this branch, it will be in Sage when #19018 will be merged.

You should have merged the branches, not cherry-picked the commit.

Nathann

comment:19 Changed 5 years ago by git

• Commit changed from 323416f9a5a46f1cc6765ef54906938353eee856 to cb0b3fbd221fb01c0e392d141ba50427b0a0294f
• Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

 ​6ed716d added a doctest ​cb0b3fb Merge branch 'reg_symm_hadamard' into t19019

comment:20 Changed 5 years ago by dimpase

please review these git things. I'll be offline for the coming 10 hours.

comment:21 Changed 5 years ago by ncohen

• Status changed from needs_review to positive_review

Looks good, thanks.

Nathann

comment:22 Changed 5 years ago by vbraun

• Branch changed from public/19019 to cb0b3fbd221fb01c0e392d141ba50427b0a0294f
• Resolution set to fixed
• Status changed from positive_review to closed

comment:23 follow-up: ↓ 24 Changed 5 years ago by dimpase

• Commit cb0b3fbd221fb01c0e392d141ba50427b0a0294f deleted

I overlooked

+    When \epsilon\in\{-1,+1\}, we say that M is a (n,\epsilon)-RSHCD if
+    M is a regular symmetric Hadamard matrix with constant diagonal
+    \delta\in\{-1,+1\} and row values all equal to \delta \epsilon
+    \sqrt(n). For more information, see [HX10]_ or 10.5.1 in
+    [BH12]_.

• what are "row values"? row sums?
• the definition of RSHCD here does not match one in [BH12], where the diagonal entries are all 1, and (n,e)-RSHCD has row sums e\sqrt{n}.
• it should be \sqrt{n}, not \sqrt(n)

And another thing - RSHCD should be mentioned in the module description in the beginning of the corr. file.

comment:24 in reply to: ↑ 23 ; follow-ups: ↓ 25 ↓ 27 Changed 5 years ago by ncohen

• what are "row values"? row sums?

Arg... Yes. Row sums.

• the definition of RSHCD here does not match one in [BH12], where the diagonal entries are all 1, and (n,e)-RSHCD has row sums e\sqrt{n}.

So what do we do? With two papers that say contradictory things?

• it should be \sqrt{n}, not \sqrt(n)

Right.

And another thing - RSHCD should be mentioned in the module description in the beginning of the corr. file.

I don't understand.

Nathann

comment:25 in reply to: ↑ 24 ; follow-up: ↓ 26 Changed 5 years ago by jmantysalo

So what do we do? With two papers that say contradictory things?

IMO select one and (almost) always document these cases. See chain_polynomial() on posets: "Warning: This is not what has been called the chain polynomial in [St1986]. The latter is - -".

comment:26 in reply to: ↑ 25 Changed 5 years ago by ncohen

IMO select one and (almost) always document these cases. See chain_polynomial() on posets: "Warning: This is not what has been called the chain polynomial in [St1986]. The latter is - -".

Jori, this was a rethoric question. The two definitions agree with each other.

comment:27 in reply to: ↑ 24 ; follow-up: ↓ 28 Changed 5 years ago by dimpase

• the definition of RSHCD here does not match one in [BH12], where the diagonal entries are all 1, and (n,e)-RSHCD has row sums e\sqrt{n}.

So what do we do? With two papers that say contradictory things?

I was comparing your definition with the one from http://homepages.cwi.nl/~aeb/math/ipm/ipm.pdf and your definition is more complicated. I did not check the other one.

And another thing - RSHCD should be mentioned in the module description in the beginning of the corr. file.

I don't understand.

If look at combinat/matrices/hadamard_matrix.py there is a paragraph saying

The module below implements the Paley constructions (see for example
[Hora]_) and the Sylvester construction. It also allows you to pull a


But there is no word about RSHCDs. Should it instead get a list of methods, etc, as more modern Sage modules have?

As well, you're not in the list of authors.

As I am gearing up for skew-Hadamard matrices, it can be done on the corresponding ticket.

comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 5 years ago by ncohen

I was comparing your definition with the one from http://homepages.cwi.nl/~aeb/math/ipm/ipm.pdf and your definition is more complicated. I did not check the other one.

I don't see how. In the paper you give the matrix is requested to have 1 on the diagonal, that's the only difference I see.

If look at combinat/matrices/hadamard_matrix.py there is a paragraph saying But there is no word about RSHCDs.

Oh, right. I'll do that.

Should it instead get a list of methods, etc, as more modern Sage modules have?

Probably. I am working on a patch related to index of functions at the moment, though. Something to make building a thematic index easier.

As well, you're not in the list of authors.

How I hate this "I am the one who did it" bullshit...

As I am gearing up for skew-Hadamard matrices, it can be done on the corresponding ticket.

Skew-hadamard? *sigh*.... Someday we will have all kind of designs in Sage. But today is not this day.

Nathann

comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 5 years ago by dimpase

I was comparing your definition with the one from http://homepages.cwi.nl/~aeb/math/ipm/ipm.pdf and your definition is more complicated. I did not check the other one.

I don't see how. In the paper you give the matrix is requested to have 1 on the diagonal, that's the only difference I see.

yes, this is the difference that makes your definition different. I suppose this is very close to the original one, as all is needed is multiplication by -1 to convert one into the other. But still not the same...

As well, you're not in the list of authors.

How I hate this "I am the one who did it" bullshit...

well, if only for uniformity...

As I am gearing up for skew-Hadamard matrices, it can be done on the corresponding ticket.

Skew-hadamard? *sigh*.... Someday we will have all kind of designs in Sage. But today is not this day.

For all practical purposes it is the same. One says that the diagonal has to be 1, the other says that it can be -1 but adapts the value of row sums. If you get one which does not have a 1 on the diagonal, then -your_matrix does the job, and all results of existence/non-existence apply.