Bug in AffineGeometryDesign

[nathanncohen]

A List is used instead of a Set, and as a result blocks are returned several times. Example :

Before

sage: print designs.AffineGeometryDesign(2, 1, GF(2)).blocks()
[[0, 1], [0, 1], [0, 2], [0, 2], [0, 3], [0, 3], [1, 2], 
 [1, 2], [1, 3], [1, 3], [2, 3], [2, 3]]

After

sage: print designs.AffineGeometryDesign(2, 1, GF(2)).blocks()
[[0, 1], [0, 2], [0, 3], [1, 2], [1, 3], [2, 3]]

Depends on #15107

Component: combinatorics

Author: Nathann Cohen

Branch/Commit: u/ncohen/15285 @ 6ff6a06

Reviewer: Stefan van Zwam

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

[nathanncohen]

Commit: 2ec76c7

[nathanncohen]

Branch: u/ncohen/15285

[nathanncohen]

New commits:

[changeset:2ec76c7]	Bug in AffineGeometryDesign
[changeset:cf71d58]	Rebase on 5.13.beta0
[changeset:9fcfb13]	Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign
[changeset:363badb]	trac 15107 -- reviewer's comments
[changeset:ee6d412]	Projective Plane designs constructor

[sagetrac-git]

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

2750b79	trac 15285: rebase on 6.1.beta4
e2935fe	Trac #15107: Rebase on 6.1.beta3
6f247f6	trac #15107: back to the first name with a new argument
8468131	trac #15285: Rebase on updated #15107

[sagetrac-git]

Changed commit from 2ec76c7 to 8468131

[fchapoton]

comment:3

Looks good, but I do not understand the doctests about

AffineGeometryDesign(3, 2, GF(2))

Why do the parameters differ from the result of is_block_design ?

Is it because it is a 3-(something) design and not a 2-(something) design ?

If so, maybe you should rather use rather BD.parameters(t=3) ?

[nathanncohen]

comment:4

Yoooooooooo !!

Looks good, but I do not understand the doctests about

AffineGeometryDesign(3, 2, GF(2))

Why do the parameters differ from the result of is_block_design ?

Is it because it is a 3-(something) design and not a 2-(something) design ?

If so, maybe you should rather use rather BD.parameters(t=3) ?

Ahahaha. Design codes are tricky :-P

The first number is the size of the sets that the design covers. That does not change.

The second number is the number of points, and that does not change.

The third number is the size of the sets contained in the design. This does not change.

The fourth number is the number of times each set to be covered is actually covered. And this one changes, because as each set was present too many times in the design, the set were covered too many times too.

But the parameter 't' has no reason to change.

Nathann

[fchapoton]

comment:5

Let me re-ask my question, trying to be more clear.

After the commits (and before also), the doctests show that

.parameters and .is_block_design give the same numbers for AffineGeometryDesign(3, 1, GF(2))

but not for AffineGeometryDesign(3, 2, GF(2))

Why is it so ? Are they supposed to always give the same result ?

[nathanncohen]

comment:6

Yooooo !

Let me re-ask my question, trying to be more clear.

After the commits (and before also), the doctests show that

.parameters and .is_block_design give the same numbers for AffineGeometryDesign(3, 1, GF(2))

but not for AffineGeometryDesign(3, 2, GF(2))

Why is it so ? Are they supposed to always give the same result ?

O_o

Well, I have absolutely no idea. I don't use this BlockDesign class. Hmmmmmm.. Let me see.

The docstring of parameters says that it does not check if the input is a block design (which can be costly). It just counts the number of blocks. And I am reading the code of is_block_design right now but I don't actually get it.

For the record

sage: IncidenceStructure([0,1],[[0,1]]*15).parameters()      
(2, 2, 2, 15)
sage: IncidenceStructure([0,1],[[0,1]]*15).is_block_design()
...
UnboundLocalError: local variable 't' referenced before assignment

And there, they do not return the same

sage: IncidenceStructure([0,1,2],[[0,1],[0,2],[1,2]]*15).is_block_design()
(True, [1, 3, 2, 15])
sage: IncidenceStructure([0,1,2],[[0,1],[0,2],[1,2]]*15).parameters()     
(2, 3, 2, 15)

Oh, I finally understand what you said ! And why you talked of this t. Well, I guess that both functions should always return the same, except when the incidence structure is not really a block design. Which parameters is not supposed to notice.

But they should agree on this t anyway. Especially if they agree on all other coordinates. And it looks like is_block_design is the one that is wrong here.

Hmmmm...

Well it seems wrong but I really do not understand what the code does. And this range(2, min(v, 11)) definitely looks like a cheap trick to not do anything if t is larger than 11...

Actually, I do not understand why this function does not call parameter to guess the value of t,v,k,lambda, then checks that the incidence structure is indeed such a design.

That's all I can say right now. I really don't understand the part of the code included in this for loop on t.

Nathann

[nathanncohen]

comment:7

I am about to send you and David an email. Looks like he is the one who wrote this code.

Nathann

[sagetrac-Stefan]

comment:8

The docstring for designs.AffineGeometryDesign is not clear. It mentions an input "v" that's not an input.

[nathanncohen]

comment:9

The docstring for designs.AffineGeometryDesign is not clear. It mentions an input "v" that's not an input.

Ahahahahah. Yes, indeed. I fix a bug in a function that is not perfectly documented, so I should fix it too.

Honestly, no problem. But no double standard either : keep the same level of expectation when you review combinat/ patches. If nobody is allowed to touch a function when it doc is not perfect this thing will be cleaned in a couple of months :-P

Nathann

[sagetrac-Stefan]

comment:10

Personally, I will. Perfection is a lofty goal, but surely a correct list of inputs, output, and a few doctests is a reasonable requirement? I'm just following the guidelines:

http://www.sagemath.org/doc/developer/trac.html#reviewing-patches

[nathanncohen]

comment:11

Personally, I will. Perfection is a lofty goal, but surely a correct list of inputs, output, and a few doctests is a reasonable requirement? I'm just following the guidelines:

You have no idea how totally I agree with you. You are right.

The point is that I have very frequent exchanges of hate mail with Nicolas (among others) about this very subject. Because there is on #10963 a comment saying "Okay, the patch is missing on the documentation side but let's consider it good as it is". Because very basic parts of the Category stuff still has no documentation, you just have to "talk to the right guys", who know what is missing (i.e. the questions they have to answer). Because I am accused of slowing down people's work in #13624 (comments 38 to 41) when I say that packages break without any reason nor explanation, and request more doc. Hell, I am told I can't complain if a package breaks for no reason because it is "experimental".

Don't misunderstand me. I totally agree with you. I just suffer of the double standard. And of feeling that I am the only one to complain constantly, while expecting myself to do this kind of work on my patches :-P

Anyway, I added a commit. I also allowed to input an integer instead of a FiniteField, because I hate to build a field to see the code discard the field and use its cardinality only.

And by the way, here is an interesting thing :

sage: 6.order()
+Infinity

Let's always leave a better place than the one we entered.

Nathann

[sagetrac-git]

Changed commit from 8468131 to 6ff6a06

[sagetrac-git]

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

6ff6a06

trac #15285: Doc of AffineGeometryDesign

[sagetrac-Stefan]

comment:14

I see your suffering. I don't interact with the combinat code usually, so I haven't encountered it. The discussion about the design parameters is unrelated to this ticket, so I'm happy to see this one go in!

[sagetrac-Stefan]

Reviewer: Stefan van Zwam

[nathanncohen]

comment:15

The discussion about the design parameters is unrelated to this ticket, so I'm happy to see this one go in!

Thank you for the review !

By the way I wrote to Frederic and David Joyner about that. David just answered and told me that Peter Cameron wrote this code. We'll end up fixing it I hope :-P

Nathann

[sagetrac-Stefan]

comment:16

Ah. Peter Cameron and I attended the same workshop this week, but I don't see him around today. Perhaps he left early, otherwise I would have asked him about it ;-)

[nathanncohen]

comment:17

The discussion about the design parameters is unrelated to this ticket, so I'm happy to see this one go in!

Now fixed in #15664.

Nathann