Opened 6 years ago

Closed 5 years ago

#15285 closed defect (fixed)

Bug in AffineGeometryDesign

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.1
Component: combinatorics Keywords:
Cc: Merged in:
Authors: Nathann Cohen Reviewers: Stefan van Zwam
Report Upstream: N/A Work issues:
Branch: u/ncohen/15285 (Commits) Commit: 6ff6a06261330d2e11aa862e1c59ac2c632a8da1
Dependencies: #15107 Stopgaps:

Description

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]]

Change History (18)

comment:1 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/15285
  • Commit set to 2ec76c72115a01be32b8b9cb65503c152d36fd8c
  • Status changed from new to needs_review

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

comment:2 Changed 6 years ago by git

  • Commit changed from 2ec76c72115a01be32b8b9cb65503c152d36fd8c to 8468131a81898d5a3ccfb38d9f1a3d44df15fe10

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

2750b79trac 15285: rebase on 6.1.beta4
e2935feTrac #15107: Rebase on 6.1.beta3
6f247f6trac #15107: back to the first name with a new argument
8468131trac #15285: Rebase on updated #15107

comment:3 follow-up: Changed 6 years ago by chapoton

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) ?

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

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

comment:5 follow-up: Changed 6 years ago by chapoton

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 ?

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

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

comment:7 Changed 6 years ago by ncohen

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

Nathann

comment:8 follow-up: Changed 6 years ago by Stefan

  • Status changed from needs_review to needs_work

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

comment:9 in reply to: ↑ 8 Changed 6 years ago by ncohen

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

comment:10 follow-up: Changed 6 years ago by Stefan

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

Last edited 6 years ago by Stefan (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 6 years ago by ncohen

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

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

comment:12 Changed 6 years ago by git

  • Commit changed from 8468131a81898d5a3ccfb38d9f1a3d44df15fe10 to 6ff6a06261330d2e11aa862e1c59ac2c632a8da1

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

6ff6a06trac #15285: Doc of AffineGeometryDesign

comment:13 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:14 follow-ups: Changed 6 years ago by Stefan

  • Reviewers set to Stefan van Zwam
  • Status changed from needs_review to positive_review

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!

comment:15 in reply to: ↑ 14 Changed 6 years ago by ncohen

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

comment:16 Changed 6 years ago by Stefan

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 ;-)

comment:17 in reply to: ↑ 14 Changed 6 years ago by ncohen

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

comment:18 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.