Opened 5 years ago

Closed 5 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 vdelecroix)

Heeeeeere they are ! ;-)

follow up: #16235

Change History (37)

comment:1 Changed 5 years ago by ncohen

  • Dependencies changed from #16248, #16231, #16269 to #16248, #16231, #16269, #16058

comment:2 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16277
  • Status changed from new to needs_review

comment:3 Changed 5 years ago by git

  • Commit set to d4f445b189ad16b28a436d9fd2fe82db53ebf3b1

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

22db72btrac #16277; Merged with #16269
2642317trac #16248: Some trivial Orthogonal Arrays
2d84806trac #16277: Merge with #16248
8b8aea8trac #16058: Organize the index of combinatorial modules
e0d2b66trac #16058: Two new categories
0293c49trac #16058: Another group
a14057bSome more groupings and separated root system types into separate list.
4b5cb2btrac #16058: Rebase on 6.2.rc0
5dbc42ftrac #16277: Merged with #16058
d4f445btrac #16277: MOLS constructions rom the Handbook of Combinatorial Designs

comment:4 Changed 5 years ago by ncohen

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 5 years ago by ncohen

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: Changed 5 years ago by vdelecroix

  • 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 5 years ago by vdelecroix

  • Description modified (diff)

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by ncohen

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

'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 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])),
...
}

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 5 years ago by vdelecroix

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 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])),
...
}

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 5 years ago by ncohen

Here it is. Gosh, it takes a lifetime to update all the doctests....

For the moment it still uses the availability keyword, and this ticket has not been merged on tpo of #16272. This is because the #16272 is not finished and I don't want to merge it before that.

Nathann

comment:11 Changed 5 years ago by git

  • Commit changed from d4f445b189ad16b28a436d9fd2fe82db53ebf3b1 to 490aa7e9c919ef8bd62d9da41489c3825ff14f72

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

490aa7etrac #16277: Reviewer's remarks

comment:12 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 5 years ago by vdelecroix

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 5 years ago by ncohen

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: Changed 5 years ago by vdelecroix

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 5 years ago by ncohen

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 5 years ago by git

  • Commit changed from 490aa7e9c919ef8bd62d9da41489c3825ff14f72 to 14420b6fa8b77896fa8f3911672979f25b3b333f

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

14420b6trac #16277: Yet another construction

comment:18 Changed 5 years ago by git

  • Commit changed from 14420b6fa8b77896fa8f3911672979f25b3b333f to 7570b39787b8e11f64317ed11f4d5c282752ac74

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0812a73trac #16281: Simplification
61dc86b16281: long comment for the construction of the projective plane
51daa7ftrac #16281: correct a doctest
e090f92trac #16272: merge #16281
9a221betrac #16272: fix doctests
5074eeetrac #16272: finer doctest to test the output of transversal_design
d81f265trac #16272: ultimate doctest
47798d2trac #16272: simplifying the structure of orthogonal_array
ddda559trac #16277: Merged with updated #16272
7570b39trac #16277: From availability to existence and broken doctests

comment:19 Changed 5 years ago by ncohen

God. There *WERE* conflicts :-PPP

Nothing bad, but still, the file was messy at first ^^;

Nathann

comment:20 Changed 5 years ago by vdelecroix

  • 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 5 years ago by git

  • Commit changed from 7570b39787b8e11f64317ed11f4d5c282752ac74 to 5e8b2af4ac6d3ab639d3ab08957459e7453447b6

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

5e8b2aftrac #16277: removes a doctests that appears several times already

comment:22 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Well, then ...

comment:23 Changed 5 years ago by ncohen

(by the way it was also tested in the TD_product docstring)

comment:24 Changed 5 years ago by git

  • 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:

e86d26ctrac #15310: Merged with 6.2.rc2
5cfee91trac #16227: Merged with updated #15310
d051cf4ValueError -> EmptySetError in latin_squares
9e5e94ftrac #16231: Merged with updated #16227
acf8988trac #16277: Merged with updated #16231

comment:25 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Still updating ...

Nathann

comment:26 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:27 Changed 5 years ago by ncohen

  • Dependencies changed from #16248, #16231, #16269, #16058 to #16248

comment:28 Changed 5 years ago by git

  • 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:

d34b012trac #16272: Merged with updated #16227
c127a6dtrac #16272: Merged with updated #16231
36a705btrac #16248: Merged with #16231
5dc4a25trac #16248: Merged with #16272
f019bd3Merge branch 'u/ncohen/16248' of trac.sagemath.org:sage into 16231
485e880trac #16277: Merged with updated #16248

comment:29 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:30 Changed 5 years ago by git

  • 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:

411a759trac #16286: Merged with updated #16277
0f3a755trac #16235: A pair of orthogonal latin squares of order 10
cb2e272trac #16235: Merged with updated #16286
aeab437trac #16235: merge the updated #16286
daa3835trac #16235: case n=1 + doc
31a53f2trac #16235: update the MOLS table
11eff2ctrac #16235: Merged with 6.2
5a0e3fetrac #16235: Merged with #16231
c0b13c4trac #16235: Merged with updated #16286
9eab3e3Merge branch 'public/16235' of trac.sagemath.org:sage into tmp

comment:31 Changed 5 years ago by ncohen

ARGGGGGGGGGGGGGG.... Wrong push T_T

comment:32 Changed 5 years ago by ncohen

Back to the previous one T_T

comment:33 Changed 5 years ago by git

  • 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 5 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:35 Changed 5 years ago by git

  • Commit changed from 485e88048f72f61d46fb61420cc5e2e91f67f117 to 270c65b335ac70bc06b5e8a901f3c4e7014f988b
  • 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:

e2749b3trac #16272: Merged with 6.3.beta0
6678079trac #16277: Merged with updated #16272
270c65btrac #16277: A typo

comment:36 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Better -_-

comment:37 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/16277 to 270c65b335ac70bc06b5e8a901f3c4e7014f988b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.