Opened 5 years ago

Closed 5 years ago

#16286 closed enhancement (fixed)

Allow k=None in MOLS/TD/OA

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorics Keywords: design, mols, orthogonal array
Cc: vdelecroix, knsam Merged in:
Authors: Nathann Cohen Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 411a759 (Commits) Commit: 411a7598bfad38b3e1542c363100a145c11cdd93
Dependencies: #16277 Stopgaps:

Description

Implements the feature we dropped in #15310, i.e. ask Sage to give us the best result it can compute.

Change History (11)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16286
  • Cc vdelecroix added
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to 5cab81cab862fd60c4a17fdd039ff22bb34b5629

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

3e5628a16281: reviewer's comment 1
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
5cab81ctrac #16286: Allow k=None in MOLS/TD/OA

comment:3 Changed 5 years ago by git

  • Commit changed from 5cab81cab862fd60c4a17fdd039ff22bb34b5629 to 4970da45741acac330b2602c967e5fed681fdfa3

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

e86d26ctrac #15310: Merged with 6.2.rc2
5cfee91trac #16227: Merged with updated #15310
d34b012trac #16272: Merged with updated #16227
4970da4trac #16286: Merged with updated #16272

comment:4 Changed 5 years ago by ncohen

  • Cc knsam added

Still updating ...

comment:5 Changed 5 years ago by vdelecroix

  • Keywords design mols orthogonal array added

adding keywords...

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

  • Status changed from needs_review to needs_work

Hi Nathann,

I run through the code and mainly changed some documentation. It is available at u/vdelecroix/16286 if you want to have a look it and possibly adopt it.

It becomes crazy: the functions return either a design , a troolean or an integer depending on the input... I added an OUTPUT section to make it clearer (only in OA/TD). Moreover I am not sure I like the NOTE inside the INPUT. On the one hand, it is unreadable in the console. On the other hand, in the html output the NOTE has a gray background and makes your eyes focus on it... but it is by far not the most important thing.

We forgot two ValueError that should be EmptySetError inside the MOLS constructor. I changed it.

Inside the TESTS of transversal design, you modified the loop to not test the case where the function should return True. As the loop is designed to test if it the case I did the change backward. Nevertheless, I added a line that checks that we got the correct value when k=None.

I added more examples and hope that what does the function looks clearer when somebody reads the documentation. In particular, most of the TESTS section are now useless and I just removed them.

Vincent

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

  • Branch changed from u/ncohen/16286 to u/vdelecroix/16286
  • Commit changed from 4970da45741acac330b2602c967e5fed681fdfa3 to a97434f3264d183056a50ac54fcb7a9912d31efc
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_work to positive_review

Yo !

I run through the code and mainly changed some documentation. It is available at u/vdelecroix/16286 if you want to have a look it and possibly adopt it.

Yep, no problem.

It becomes crazy: the functions return either a design , a troolean or an integer depending on the input...

True, but I think that without reading the doc you can guess what you will find, or at least correctly interpret the output. So well, doesn't matter ;-)

Besides, with the doctest you quickly see what it means.

I added an OUTPUT section to make it clearer (only in OA/TD). Moreover I am not sure I like the NOTE inside the INPUT. On the one hand, it is unreadable in the console. On the other hand, in the html output the NOTE has a gray background and makes your eyes focus on it... but it is by far not the most important thing.

Ahahaahah. Well, to be honest I just want all these patches to be merged, then we will clean stuff. Each time you add a doctest I have to rewrite it one thousand times because the new constructions break it. It's over-documented and over-tested at the moment I believe, but it is not a problem. We have time.

We forgot two ValueError that should be EmptySetError inside the MOLS constructor. I changed it.

Thanks ! I am still not used to those EmptySetError

Inside the TESTS of transversal design, you modified the loop to not test the case where the function should return True. As the loop is designed to test if it the case I did the change backward. Nevertheless, I added a line that checks that we got the correct value when k=None.

You are testing a loop ? Yes of course, everything can fail.

I added more examples and hope that what does the function looks clearer when somebody reads the documentation. In particular, most of the TESTS section are now useless and I just removed them.

OKayyyyyyyy. Well for me it is good to go, too. I am eager to see it stabilize, right now I have so many versions of this code in my head that I barely know what is already there and what is left to implement.

Thanks !

Nathann


New commits:

a97434ftrac #16286: more doc, more tests

comment:8 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 5 years ago by ncohen

  • Dependencies changed from #16272 to #16277

comment:10 Changed 5 years ago by ncohen

  • Branch changed from u/vdelecroix/16286 to public/16286
  • Commit changed from a97434f3264d183056a50ac54fcb7a9912d31efc to 411a7598bfad38b3e1542c363100a145c11cdd93

Last 10 new commits:

5dbc42ftrac #16277: Merged with #16058
d4f445btrac #16277: MOLS constructions rom the Handbook of Combinatorial Designs
490aa7etrac #16277: Reviewer's remarks
14420b6trac #16277: Yet another construction
ddda559trac #16277: Merged with updated #16272
7570b39trac #16277: From availability to existence and broken doctests
5e8b2aftrac #16277: removes a doctests that appears several times already
acf8988trac #16277: Merged with updated #16231
485e880trac #16277: Merged with updated #16248
411a759trac #16286: Merged with updated #16277

comment:11 Changed 5 years ago by vbraun

  • Branch changed from public/16286 to 411a7598bfad38b3e1542c363100a145c11cdd93
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.