Opened 6 years ago
Closed 6 years ago
#16277 closed enhancement (fixed)
MOLS constructions rom the Handbook of Combinatorial Designs
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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 6 years ago by
- Dependencies changed from #16248, #16231, #16269 to #16248, #16231, #16269, #16058
comment:2 Changed 6 years ago by
- Branch set to u/ncohen/16277
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Commit set to d4f445b189ad16b28a436d9fd2fe82db53ebf3b1
comment:4 Changed 6 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 6 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 follow-up: ↓ 8 Changed 6 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) (semi-stupid 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 6 years ago by
- Description modified (diff)
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 9 Changed 6 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) (semi-stupid 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 6 years ago by
Replying to ncohen:
4) (semi-stupid 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 6 years ago by
comment:11 Changed 6 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 6 years ago by
- Status changed from needs_work to needs_review
comment:13 follow-up: ↓ 14 Changed 6 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 6 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 follow-up: ↓ 16 Changed 6 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 6 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 6 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 6 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 6 years ago by
God. There *WERE* conflicts :-PPP
Nothing bad, but still, the file was messy at first ^^;
Nathann
comment:20 Changed 6 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 6 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 6 years ago by
- Status changed from needs_review to positive_review
Well, then ...
comment:23 Changed 6 years ago by
(by the way it was also tested in the TD_product
docstring)
comment:24 Changed 6 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 6 years ago by
- Status changed from needs_review to positive_review
Still updating ...
Nathann
comment:26 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:27 Changed 6 years ago by
- Dependencies changed from #16248, #16231, #16269, #16058 to #16248
comment:28 Changed 6 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 6 years ago by
- Status changed from needs_review to positive_review
comment:30 Changed 6 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 6 years ago by
ARGGGGGGGGGGGGGG.... Wrong push T_T
comment:32 Changed 6 years ago by
Back to the previous one T_T
comment:33 Changed 6 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 6 years ago by
- Status changed from needs_review to positive_review
comment:35 Changed 6 years ago by
- Commit changed from 485e88048f72f61d46fb61420cc5e2e91f67f117 to 270c65b335ac70bc06b5e8a901f3c4e7014f988b
- Status changed from positive_review to needs_review
comment:37 Changed 6 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