Opened 7 years ago
Closed 7 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 7 years ago by
- Branch set to u/ncohen/15285
- Commit set to 2ec76c72115a01be32b8b9cb65503c152d36fd8c
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Commit changed from 2ec76c72115a01be32b8b9cb65503c152d36fd8c to 8468131a81898d5a3ccfb38d9f1a3d44df15fe10
comment:3 follow-up: ↓ 4 Changed 7 years ago by
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 7 years ago by
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: ↓ 6 Changed 7 years ago by
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 7 years ago by
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 forAffineGeometryDesign(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 7 years ago by
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: ↓ 9 Changed 7 years ago by
- 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 7 years ago by
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: ↓ 11 Changed 7 years ago by
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
comment:11 in reply to: ↑ 10 Changed 7 years ago by
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" (and I see that someone just volunteered to write it for him because it wouldn't happen otherwise). 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
comment:12 Changed 7 years ago by
- Commit changed from 8468131a81898d5a3ccfb38d9f1a3d44df15fe10 to 6ff6a06261330d2e11aa862e1c59ac2c632a8da1
Branch pushed to git repo; I updated commit sha1. New commits:
6ff6a06 | trac #15285: Doc of AffineGeometryDesign
|
comment:13 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:14 follow-ups: ↓ 15 ↓ 17 Changed 7 years ago by
- 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 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
New commits: