Opened 7 years ago
Closed 7 years ago
#16277 closed enhancement (fixed)
MOLS constructions rom the Handbook of Combinatorial Designs
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  combinatorics  Keywords:  
Cc:  vdelecroix, brett  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  270c65b (Commits)  Commit:  270c65b335ac70bc06b5e8a901f3c4e7014f988b 
Dependencies:  #16248  Stopgaps: 
Description (last modified by )
Heeeeeere they are ! ;)
follow up: #16235
Change History (37)
comment:1 Changed 7 years ago by
 Dependencies changed from #16248, #16231, #16269 to #16248, #16231, #16269, #16058
comment:2 Changed 7 years ago by
 Branch set to u/ncohen/16277
 Status changed from new to needs_review
comment:3 Changed 7 years ago by
 Commit set to d4f445b189ad16b28a436d9fd2fe82db53ebf3b1
comment:4 Changed 7 years ago by
Only the last commit implements something. All others are dependencies.
God, I can't say how glad I am to see this ticket created :P
Nathann
comment:5 Changed 7 years ago by
By the way Vincent : the database takes 30s to test on my computer... Do we make those tests long tests ?
Nathann
comment:6 followup: ↓ 8 Changed 7 years ago by
 Status changed from needs_review to needs_work
Hi Nathann,
Globally looks good. Documentation builds and all test pass.
1) Could we make it go after #16272? There is a moderately trivial rebase to do. I think it would be easier in that direction.
2) Could we also move the TD_6_12
into the database module?
3) In the doctests of database, I would rather call directly the function themselves followed by is_transversal_design
. We have no idea (at least me) that the corresponding design is actually returned when we call designs.orthogonal_array
with the good parameters. Nevertheless it is a good test to check that the latter actually return a value for those parameters (and to see whether or not it is the corresponding value).
4) (semistupid remark) For the 6 construction of OA from Vmt, the function is rather trivial. One way to avoid duplication is to modify the dictionnary into n > (k, constructor, extra_args)
. For the Vmt the entries would look like
OA_constructions = { ... 46: (6, OA_from_Vmt, (4,9,[0, 1, 3, 2, 8])), 50: (8, OA_from_Vmt, (6,7,[0, 1, 3, 16, 35, 26, 36])), ... }
Vincent
comment:7 Changed 7 years ago by
 Description modified (diff)
comment:8 in reply to: ↑ 6 ; followup: ↓ 9 Changed 7 years ago by
Yo !
1) Could we make it go after #16272? There is a moderately trivial rebase to do. I think it would be easier in that direction.
Yeah, no problem I guess.
2) Could we also move the
TD_6_12
into the database module?
Yepyep, I will do that.
3) In the doctests of database, I would rather call directly the function themselves followed by
is_transversal_design
. We have no idea (at least me) that the corresponding design is actually returned when we calldesigns.orthogonal_array
with the good parameters. Nevertheless it is a good test to check that the latter actually return a value for those parameters (and to see whether or not it is the corresponding value).
'f you want ......
4) (semistupid remark) For the 6 construction of OA from Vmt, the function is rather trivial. One way to avoid duplication is to modify the dictionnary into
n > (k, constructor, extra_args)
. For the Vmt the entries would look likeOA_constructions = { ... 46: (6, OA_from_Vmt, (4,9,[0, 1, 3, 2, 8])), 50: (8, OA_from_Vmt, (6,7,[0, 1, 3, 16, 35, 26, 36])), ... }
If we do that the OA will be built when Sage starts. Plus I would like to thank Julian R. Abel somewhere.
Nathann
comment:9 in reply to: ↑ 8 Changed 7 years ago by
Replying to ncohen:
4) (semistupid remark) For the 6 construction of OA from Vmt, the function is rather trivial. One way to avoid duplication is to modify the dictionnary into
n > (k, constructor, extra_args)
. For the Vmt the entries would look likeOA_constructions = { ... 46: (6, OA_from_Vmt, (4,9,[0, 1, 3, 2, 8])), 50: (8, OA_from_Vmt, (6,7,[0, 1, 3, 16, 35, 26, 36])), ... }If we do that the OA will be built when Sage starts. Plus I would like to thank Julian R. Abel somewhere.
Agreed. And it is better to test the corresponding code in the docstring of the function.
comment:10 Changed 7 years ago by
comment:11 Changed 7 years ago by
 Commit changed from d4f445b189ad16b28a436d9fd2fe82db53ebf3b1 to 490aa7e9c919ef8bd62d9da41489c3825ff14f72
Branch pushed to git repo; I updated commit sha1. New commits:
490aa7e  trac #16277: Reviewer's remarks

comment:12 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:13 followup: ↓ 14 Changed 7 years ago by
Hi Nathann,
In the doctest, I would like to see:
sage: TD = TD_6_12() sage: TDbis = designs.transversal_design(6,12) sage: TD == TDbis True
It is worth it to see if the individual designs are actually used in the construction, isn't it? Even if the answer is False
at some point, it still makes sense to keep the function and the doctest.
I do not mind too much as it is painful to edit all doctests in database.py
.
Vincent
comment:14 in reply to: ↑ 13 Changed 7 years ago by
In the doctest, I would like to see:
Hey man, what you "would like to see" takes me 20 minutes each time....
sage: TD = TD_6_12() sage: TDbis = designs.transversal_design(6,12) sage: TD == TDbis True
WHAT IS THE POIIIIIIIIIIIIIIIINT ????
It is worth it to see if the individual designs are actually used in the construction, isn't it? Even if the answer is
False
at some point, it still makes sense to keep the function and the doctest.
Who cares ? The function checks that the designs returned are good, and it checks that they are available through the main entry point. It already takes 30 seconds to check this file ! You want to double it to check ... that ?
Nathann
comment:15 followup: ↓ 16 Changed 7 years ago by
As I said, I don't mind as I know that it is painful to edit.
The point is that in the future, somebody might come and ask: for a given k,n how many different TD(k,n) do I have? But an equality test is far from an isomorphism test, so please leave it.
Vincent
comment:16 in reply to: ↑ 15 Changed 7 years ago by
The point is that in the future, somebody might come and ask: for a given k,n how many different TD(k,n) do I have? But an equality test is far from an isomorphism test, so please leave it.
......
Yeah, I guess it is a bit too early to do things like that ...
Nathann
comment:17 Changed 7 years ago by
 Commit changed from 490aa7e9c919ef8bd62d9da41489c3825ff14f72 to 14420b6fa8b77896fa8f3911672979f25b3b333f
Branch pushed to git repo; I updated commit sha1. New commits:
14420b6  trac #16277: Yet another construction

comment:18 Changed 7 years ago by
 Commit changed from 14420b6fa8b77896fa8f3911672979f25b3b333f to 7570b39787b8e11f64317ed11f4d5c282752ac74
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
0812a73  trac #16281: Simplification

61dc86b  16281: long comment for the construction of the projective plane

51daa7f  trac #16281: correct a doctest

e090f92  trac #16272: merge #16281

9a221be  trac #16272: fix doctests

5074eee  trac #16272: finer doctest to test the output of transversal_design

d81f265  trac #16272: ultimate doctest

47798d2  trac #16272: simplifying the structure of orthogonal_array

ddda559  trac #16277: Merged with updated #16272

7570b39  trac #16277: From availability to existence and broken doctests

comment:19 Changed 7 years ago by
God. There *WERE* conflicts :PPP
Nothing bad, but still, the file was messy at first ^^;
Nathann
comment:20 Changed 7 years ago by
 Reviewers set to Vincent Delecroix
Hi Nathann,
In orthogonal_arrays.py
in the function transversal_design
you added a test
`TD(6,12)` :: sage: _ = designs.transversal_design(6,12)
which is actually already tested in the block above it and in database.py
. So if you don't mind, you can simply remove it.
Otherwise, everything is fine and it could go to positive review after that!
Vincent
comment:21 Changed 7 years ago by
 Commit changed from 7570b39787b8e11f64317ed11f4d5c282752ac74 to 5e8b2af4ac6d3ab639d3ab08957459e7453447b6
Branch pushed to git repo; I updated commit sha1. New commits:
5e8b2af  trac #16277: removes a doctests that appears several times already

comment:22 Changed 7 years ago by
 Status changed from needs_review to positive_review
Well, then ...
comment:23 Changed 7 years ago by
(by the way it was also tested in the TD_product
docstring)
comment:24 Changed 7 years ago by
 Commit changed from 5e8b2af4ac6d3ab639d3ab08957459e7453447b6 to acf898895aa13b3ae7b66dc5db00c3d8ed9889a7
 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:
e86d26c  trac #15310: Merged with 6.2.rc2

5cfee91  trac #16227: Merged with updated #15310

d051cf4  ValueError > EmptySetError in latin_squares

9e5e94f  trac #16231: Merged with updated #16227

acf8988  trac #16277: Merged with updated #16231

comment:25 Changed 7 years ago by
 Status changed from needs_review to positive_review
Still updating ...
Nathann
comment:26 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:27 Changed 7 years ago by
 Dependencies changed from #16248, #16231, #16269, #16058 to #16248
comment:28 Changed 7 years ago by
 Commit changed from acf898895aa13b3ae7b66dc5db00c3d8ed9889a7 to 485e88048f72f61d46fb61420cc5e2e91f67f117
 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:
d34b012  trac #16272: Merged with updated #16227

c127a6d  trac #16272: Merged with updated #16231

36a705b  trac #16248: Merged with #16231

5dc4a25  trac #16248: Merged with #16272

f019bd3  Merge branch 'u/ncohen/16248' of trac.sagemath.org:sage into 16231

485e880  trac #16277: Merged with updated #16248

comment:29 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:30 Changed 7 years ago by
 Commit changed from 485e88048f72f61d46fb61420cc5e2e91f67f117 to 9eab3e3b8b3f624090cebed5f8e7d4bdf5431d51
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
411a759  trac #16286: Merged with updated #16277

0f3a755  trac #16235: A pair of orthogonal latin squares of order 10

cb2e272  trac #16235: Merged with updated #16286

aeab437  trac #16235: merge the updated #16286

daa3835  trac #16235: case n=1 + doc

31a53f2  trac #16235: update the MOLS table

11eff2c  trac #16235: Merged with 6.2

5a0e3fe  trac #16235: Merged with #16231

c0b13c4  trac #16235: Merged with updated #16286

9eab3e3  Merge branch 'public/16235' of trac.sagemath.org:sage into tmp

comment:31 Changed 7 years ago by
ARGGGGGGGGGGGGGG.... Wrong push T_T
comment:32 Changed 7 years ago by
Back to the previous one T_T
comment:33 Changed 7 years ago by
 Commit changed from 9eab3e3b8b3f624090cebed5f8e7d4bdf5431d51 to 485e88048f72f61d46fb61420cc5e2e91f67f117
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:34 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:35 Changed 7 years ago by
 Commit changed from 485e88048f72f61d46fb61420cc5e2e91f67f117 to 270c65b335ac70bc06b5e8a901f3c4e7014f988b
 Status changed from positive_review to needs_review
comment:37 Changed 7 years ago by
 Branch changed from u/ncohen/16277 to 270c65b335ac70bc06b5e8a901f3c4e7014f988b
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
trac #16277; Merged with #16269
trac #16248: Some trivial Orthogonal Arrays
trac #16277: Merge with #16248
trac #16058: Organize the index of combinatorial modules
trac #16058: Two new categories
trac #16058: Another group
Some more groupings and separated root system types into separate list.
trac #16058: Rebase on 6.2.rc0
trac #16277: Merged with #16058
trac #16277: MOLS constructions rom the Handbook of Combinatorial Designs