Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16535 closed enhancement (fixed)

get rid of who_asked parameter in combinatorial design and move Wilson constructions

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.3
Component: combinatorial designs Keywords:
Cc: ncohen, brett Merged in:
Authors: Vincent Delecroix Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 2b3c809 (Commits) Commit:
Dependencies: #16528 Stopgaps:

Description (last modified by vdelecroix)

1) Moving the three Wilson constructions (product, one truncated group and two truncated groups) from orthogonal_arrays.py to orthogonal_arrays_recursive.py will improve readability.

2) There is no need of the who_asked parameter in the functions orthogonal_array, mutually_orthogonal_latin_squares and transversal_design since all recursive functions belong to the orthogonal_array.

3) As remarked in #16503, avoid calling recursive construction for OA(3,n).

Change History (45)

comment:1 Changed 5 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from get rid of who_asked parameter in combinatorial design to get rid of who_asked parameter in combinatorial design and move Wilson constructions

comment:2 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/16535
  • Commit set to 67422b8a3c5554ed978dd9afcf37473b07a7c32e
  • Description modified (diff)

Last 10 new commits:

0a7d853trac #16423: Broken doctests
b329351trac #16499: Cheap speedup in the OA recursive constructions
a67c04ftrac #16500: New recursive constructions of Orthogonal Arrays
41c50d5trac #16500: Simplified find_recursive_construction
e1992cetrac #16500: doc + speedup
697dd0ctrac #16500: Typoes in the doc
71dad5dtrac #16503: q-x construction of Orthogonal Arrays
91eb3d2trac #16503: Review
2383681trac #16503: cut the loop
67422b8trac #16535: mv W. cons. to orthogonal_arrays_recursive.py

comment:4 Changed 5 years ago by git

  • Commit changed from 67422b8a3c5554ed978dd9afcf37473b07a7c32e to 4eab09bda84b707ae44909a5336892de44d2c49b

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

e948cf6trac #16423: Aligning the alignment
0a7d853trac #16423: Broken doctests
b329351trac #16499: Cheap speedup in the OA recursive constructions
a67c04ftrac #16500: New recursive constructions of Orthogonal Arrays
41c50d5trac #16500: Simplified find_recursive_construction
e1992cetrac #16500: doc + speedup
697dd0ctrac #16500: Typoes in the doc
71dad5dtrac #16503: q-x construction of Orthogonal Arrays
91eb3d2trac #16503: Review
4eab09btrac #16535: mv W. cons. to orthogonal_arrays_recursive.py

comment:5 Changed 5 years ago by vdelecroix

Ok. first step done.

comment:6 Changed 5 years ago by ncohen

This will be in conflict with #16559.

Nathan

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

Ok, not a too big conflict. The hardest part was not the changes to wilson_construction. Do you agree on having master_design=None in wilson_construction (see my commit)?

Vincent

comment:8 Changed 5 years ago by git

  • Commit changed from 4eab09bda84b707ae44909a5336892de44d2c49b to 3794b9f87708d14754cb3d90a1b631d41cb0fea1

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

3794b9ftrac #16535: remove who_asked

comment:9 Changed 5 years ago by git

  • Commit changed from 3794b9f87708d14754cb3d90a1b631d41cb0fea1 to dd3f9ea107fdeda8c0660f0909055dfa85ded145

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

dd3f9eatrac #16535: avoid calling recursive construction in OA(3,n)

comment:10 Changed 5 years ago by vdelecroix

  • Status changed from new to needs_review

Ok... much better now. Building the MOLS is even faster!

Vincent

comment:11 Changed 5 years ago by ncohen

The last commit goes in another ticket. Raz le cul des 9000 modifs sans aucun lien dans le même patch.

Nathann

comment:12 Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:13 Changed 5 years ago by ncohen

Bon ce truc peut rester, ca prend que 3 lignes.

Par contre ca va grave attendre demain.

Nathann

comment:14 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

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

Ok, not a too big conflict. The hardest part was not the changes to wilson_construction. Do you agree on having master_design=None in wilson_construction (see my commit)?

It's a good idea but it is 1) unrelated to this ticket 2) in conflict with the already tricky #16559. Thus it would be better to do this change after that ticket.

Nathann

comment:16 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_work

There is no reason to handle product decomposition and the find_wilson_* functions differently from the other constructions.

Nathann

comment:17 Changed 5 years ago by git

  • Commit changed from dd3f9ea107fdeda8c0660f0909055dfa85ded145 to c376ec1a3eb5b98f0a7d4371c18bd9e7f4839bce

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

c376ec1trac #16535: let wilson_construction unchanged

comment:18 in reply to: ↑ 16 Changed 5 years ago by vdelecroix

Replying to ncohen:

There is no reason to handle product decomposition and the find_wilson_* functions differently from the other constructions.

Right. So I implemented a function that we will have to remove as soon as wilson_decomposition accept None as a master design... It will also help the merge with #16559

Needs review.

comment:19 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:20 Changed 5 years ago by vdelecroix

And I rewrote history at u/vdelecroix/16535v2.

EDIT: I loooooove git rebase -i !!

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:21 Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_work

Yo !

  • You can add n1<=n2 in find_product
  • This should be a link to the constructor's doc
+    - Wilson constructions with 0, 1 or 2 truncated groups
     - :func:`construction_3_3`

Nathann

comment:22 Changed 5 years ago by vdelecroix

  • Branch changed from u/vdelecroix/16535 to u/vdelecroix/16535v2
  • Commit changed from c376ec1a3eb5b98f0a7d4371c18bd9e7f4839bce to b5b480db665c488982b811c851077252d36a871f
  • Status changed from needs_work to needs_review

New commits:

15665e1trac #16535: mv W. cons. to orthogonal_arrays_recursive.py
9c39549trac #16535: remove who_asked
6a51243trac #16535: avoid calling recursive construction in OA(3,n)
b5b480dtrac #16535: n1 <= n2 in product decomp. + better doc

comment:23 Changed 5 years ago by ncohen

  • to the Wilson's --> to Wilson's
  • :func:simple_wilson_construction (with 0,1, or 2 truncated columns)
  • - ``k,n`` (integers) -- see above. stop making style changes. You don't like my style, I don't like yours, but mine was there first so respect that. Otherwise we will just stop changing stuff to our linking.

By the way #16503 must be reviewed first.

Nathann

comment:24 Changed 5 years ago by git

  • Commit changed from b5b480db665c488982b811c851077252d36a871f to 1f87955173aaad6aa501f838a5dcf467bbe6ebf6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1f87955trac #16535: n1 <= n2 in product decomp. + better doc

comment:25 follow-up: Changed 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Goooooooooooooood to go !

Nathann

comment:26 in reply to: ↑ 25 Changed 5 years ago by vdelecroix

Replying to ncohen:

Goooooooooooooood to go !

Thanks!

comment:27 Changed 5 years ago by ncohen

This branch is in conflict with both #16524 and #16528

comment:28 Changed 5 years ago by ncohen

  • Status changed from positive_review to needs_work

comment:29 Changed 5 years ago by git

  • Commit changed from 1f87955173aaad6aa501f838a5dcf467bbe6ebf6 to 2b3c809a5771d9a8c04a985a3314ae4d85a839ef

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

8da2c73trac #16503: Broken doc
0d768dftrac #16535: merge #16503
002ceeetrac #16524: OA(9,135)
c6e78c8trac #16524: move the cyclic difference set to the constructor
a0294d3trac #16524: more comments in the doc
41b7f00trac #16535: merge #16524
3487f09trac #16528: OA(9,120)
c80b7f4trac #16528: review
2b3c809trac #16535: merge #16528

comment:30 Changed 5 years ago by vdelecroix

  • Dependencies changed from #16503 to #16528
  • Status changed from needs_work to needs_review

comment:31 Changed 5 years ago by ncohen

Ahahahahah. Yeah, you are now a big fan of history rewrite :-P

Nathann

comment:32 Changed 5 years ago by ncohen

Oh, no it is not a rebase but a merge.. I thought it was, given the number of commits :-D

Nathann

comment:33 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

branch okay + html doc ok + pdf doc okay !!!

Nathann

comment:34 Changed 5 years ago by vdelecroix

Cool, thanks!

Vincent

comment:35 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

doctests not okay!!! ;-)

sage -t --long src/sage/combinat/designs/database.py
**********************************************************************
File "src/sage/combinat/designs/database.py", line 3084, in sage.combinat.designs.database.RBIBD_120_8_1
Failed example:
    _ = designs.BalancedIncompleteBlockDesign(120,8)
Expected nothing
Got:
    doctest:1: DeprecationWarning: BalancedIncompleteBlockDesign is deprecated. Please use sage.combinat.designs.bibd.balanced_incomplete_block_design instead.
    See http://trac.sagemath.org/16446 for details.

comment:36 Changed 5 years ago by ncohen

HMmmmm.... Volker, it woud be cool if you could give us the releases you work with. For us, when it's closed we can "forget about it" but then you come and tell us that it needs to be rebased on a ticket that does not appear anywhere on trac ... This problems comes from #16446.

Nathann

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

This should be corrected in #16528 and not here.

comment:38 in reply to: ↑ 37 Changed 5 years ago by ncohen

This should be corrected in #16528 and not here.

True ! I will add a commit.

Nathann

comment:39 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_work to positive_review

Okay, well. Then back to positive_review, as the problem has been fixed in #16528.

Nathann

comment:40 in reply to: ↑ 39 Changed 5 years ago by vdelecroix

Replying to ncohen:

Okay, well. Then back to positive_review, as the problem has been fixed in #16528.

Thanks for handling this... and you are right: it is hard to take care of closed ticket not in the development release! Would be cool if we had access to the alpha branch with all the closed tickets merged.

Vincent

comment:41 follow-up: Changed 5 years ago by vbraun

If you deprecate something then you should have the foresight to not use it in your current development. There is no "alpha" branch, only a work in progress that almost certainly will have parts of its history rewritten until the next beta is ready.

comment:42 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/16535v2 to 2b3c809a5771d9a8c04a985a3314ae4d85a839ef
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:43 in reply to: ↑ 41 Changed 5 years ago by ncohen

  • Commit 2b3c809a5771d9a8c04a985a3314ae4d85a839ef deleted

If you deprecate something then you should have the foresight to not use it in your current development.

What if you learnt that that patch was written before the one which deprecated this function ?

Nathann

comment:44 follow-up: Changed 5 years ago by vbraun

The chronological order doesn't really matter. You could have merged this (older) ticket into the branch that deprecates the function. And there is of course no law against going back to the older branch and avoiding the deprecated functionality in the first place. Really we are talking about a ~2 week time window until the next beta release. There will always be rare cases where branches that are about to be merged conflict, and if you forgot that you authored the conflict yourself then the release manager will remind you ;-)

comment:45 in reply to: ↑ 44 Changed 5 years ago by ncohen

Replying to vbraun:

The chronological order doesn't really matter.

Your previous comment was the following

If you deprecate something then you should have the foresight to not use it in your current development.

I explained to you why it was groundless.

We just try to add code to Sage, and we try to ensure it passes tests at all times, and the doc, but there is a lot of commits involved and building the pdf doc, for instance, is not instantaneous. I also wanted to tell you how closing tickets without making a release after can complicate our work, and so I did.

Understand what you will.

Nathann

Last edited 5 years ago by ncohen (previous) (diff)
Note: See TracTickets for help on using tickets.