#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 )
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 7 years ago by
- 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 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
- Branch set to u/vdelecroix/16535
- Commit set to 67422b8a3c5554ed978dd9afcf37473b07a7c32e
- Description modified (diff)
comment:4 Changed 7 years ago by
- Commit changed from 67422b8a3c5554ed978dd9afcf37473b07a7c32e to 4eab09bda84b707ae44909a5336892de44d2c49b
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e948cf6 | trac #16423: Aligning the alignment
|
0a7d853 | trac #16423: Broken doctests
|
b329351 | trac #16499: Cheap speedup in the OA recursive constructions
|
a67c04f | trac #16500: New recursive constructions of Orthogonal Arrays
|
41c50d5 | trac #16500: Simplified find_recursive_construction
|
e1992ce | trac #16500: doc + speedup
|
697dd0c | trac #16500: Typoes in the doc
|
71dad5d | trac #16503: q-x construction of Orthogonal Arrays
|
91eb3d2 | trac #16503: Review
|
4eab09b | trac #16535: mv W. cons. to orthogonal_arrays_recursive.py
|
comment:5 Changed 7 years ago by
Ok. first step done.
comment:6 Changed 7 years ago by
This will be in conflict with #16559.
Nathan
comment:7 follow-up: ↓ 15 Changed 7 years ago by
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 7 years ago by
- Commit changed from 4eab09bda84b707ae44909a5336892de44d2c49b to 3794b9f87708d14754cb3d90a1b631d41cb0fea1
Branch pushed to git repo; I updated commit sha1. New commits:
3794b9f | trac #16535: remove who_asked
|
comment:9 Changed 7 years ago by
- Commit changed from 3794b9f87708d14754cb3d90a1b631d41cb0fea1 to dd3f9ea107fdeda8c0660f0909055dfa85ded145
Branch pushed to git repo; I updated commit sha1. New commits:
dd3f9ea | trac #16535: avoid calling recursive construction in OA(3,n)
|
comment:10 Changed 7 years ago by
- Status changed from new to needs_review
Ok... much better now. Building the MOLS is even faster!
Vincent
comment:11 Changed 7 years ago by
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 7 years ago by
- Status changed from needs_review to needs_work
comment:13 Changed 7 years ago by
Bon ce truc peut rester, ca prend que 3 lignes.
Par contre ca va grave attendre demain.
Nathann
comment:14 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:15 in reply to: ↑ 7 Changed 7 years ago by
Ok, not a too big conflict. The hardest part was not the changes to
wilson_construction
. Do you agree on havingmaster_design=None
inwilson_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: ↓ 18 Changed 7 years ago by
- 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 7 years ago by
- Commit changed from dd3f9ea107fdeda8c0660f0909055dfa85ded145 to c376ec1a3eb5b98f0a7d4371c18bd9e7f4839bce
Branch pushed to git repo; I updated commit sha1. New commits:
c376ec1 | trac #16535: let wilson_construction unchanged
|
comment:18 in reply to: ↑ 16 Changed 7 years ago by
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 7 years ago by
- Status changed from needs_work to needs_review
comment:20 Changed 7 years ago by
And I rewrote history at u/vdelecroix/16535v2
.
EDIT: I loooooove git rebase -i
!!
comment:21 Changed 7 years ago by
- Status changed from needs_review to needs_work
Yo !
- You can add
n1<=n2
infind_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 7 years ago by
- Branch changed from u/vdelecroix/16535 to u/vdelecroix/16535v2
- Commit changed from c376ec1a3eb5b98f0a7d4371c18bd9e7f4839bce to b5b480db665c488982b811c851077252d36a871f
- Status changed from needs_work to needs_review
comment:23 Changed 7 years ago by
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 7 years ago by
- Commit changed from b5b480db665c488982b811c851077252d36a871f to 1f87955173aaad6aa501f838a5dcf467bbe6ebf6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1f87955 | trac #16535: n1 <= n2 in product decomp. + better doc
|
comment:25 follow-up: ↓ 26 Changed 7 years ago by
- Reviewers set to Nathann Cohen
- Status changed from needs_review to positive_review
Goooooooooooooood to go !
Nathann
comment:26 in reply to: ↑ 25 Changed 7 years ago by
comment:27 Changed 7 years ago by
comment:28 Changed 7 years ago by
- Status changed from positive_review to needs_work
comment:29 Changed 7 years ago by
- Commit changed from 1f87955173aaad6aa501f838a5dcf467bbe6ebf6 to 2b3c809a5771d9a8c04a985a3314ae4d85a839ef
Branch pushed to git repo; I updated commit sha1. New commits:
8da2c73 | trac #16503: Broken doc
|
0d768df | trac #16535: merge #16503
|
002ceee | trac #16524: OA(9,135)
|
c6e78c8 | trac #16524: move the cyclic difference set to the constructor
|
a0294d3 | trac #16524: more comments in the doc
|
41b7f00 | trac #16535: merge #16524
|
3487f09 | trac #16528: OA(9,120)
|
c80b7f4 | trac #16528: review
|
2b3c809 | trac #16535: merge #16528
|
comment:30 Changed 7 years ago by
- Dependencies changed from #16503 to #16528
- Status changed from needs_work to needs_review
comment:31 Changed 7 years ago by
Ahahahahah. Yeah, you are now a big fan of history rewrite :-P
Nathann
comment:32 Changed 7 years ago by
Oh, no it is not a rebase but a merge.. I thought it was, given the number of commits :-D
Nathann
comment:33 Changed 7 years ago by
- Status changed from needs_review to positive_review
branch okay + html doc ok + pdf doc okay !!!
Nathann
comment:34 Changed 7 years ago by
Cool, thanks!
Vincent
comment:35 Changed 7 years ago by
- 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 7 years ago by
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: ↓ 38 Changed 7 years ago by
This should be corrected in #16528 and not here.
comment:38 in reply to: ↑ 37 Changed 7 years ago by
comment:39 follow-up: ↓ 40 Changed 7 years ago by
- 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 7 years ago by
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: ↓ 43 Changed 7 years ago by
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 7 years ago by
- 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 7 years ago by
- 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: ↓ 45 Changed 7 years ago by
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 7 years ago by
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 10 new commits:
trac #16423: Broken doctests
trac #16499: Cheap speedup in the OA recursive constructions
trac #16500: New recursive constructions of Orthogonal Arrays
trac #16500: Simplified find_recursive_construction
trac #16500: doc + speedup
trac #16500: Typoes in the doc
trac #16503: q-x construction of Orthogonal Arrays
trac #16503: Review
trac #16503: cut the loop
trac #16535: mv W. cons. to orthogonal_arrays_recursive.py