Opened 7 years ago

Closed 7 years ago

# redesign transversal designs

Reported by: Owned by: vdelecroix Vincent Delecroix major sage-6.3 combinatorics designs, orthogona arrays ncohen, brett Nathann Cohen, Vincent Delecroix Nathann Cohen, Vincent Delecroix N/A e2749b3 (Commits) e2749b3db4cd78fc5e9ea8219d4cdf895b283465 #16231

The tickets #15310 and #16227 introduce a nice availability keywords to the function transversal_design. With availability=True the return value is the answer to the question "Does Sage know how to build a TD(k,n)"? Using Unknown from sage.misc.unknown we can turn the question into "Do we know mathematically that a TD(k,n) exist?" whose answer would be:

• True if Sage knows how to do it
• Unknwon if neither Sage nor mathematics can help
• False if we know mathematically that such construction does not exist

As the semantic changes, we will also turn the keyword availability into existence (or maybe have both).

In the same ticket, we will include some of the known non existence of transversal designs:

• The Bruck Ryser Chowla Theorm gives the non-existence of many TD(n+1,n)
• C. Lam in Lam, "The Search for a Finite Projective Plane of Order 10" (1991) proved that TD(11,10) cannot exist

And we might see later

• there is some work (?) that shows that if $k$ is large enough then the existence of TD(k,n) implies the existence of TD(n+1,n) and so the non-existence results can percolate downwards in k

### comment:1 Changed 7 years ago by vdelecroix

• Description modified (diff)

### comment:2 Changed 7 years ago by vdelecroix

• Dependencies set to #15310, #16227

### comment:3 Changed 7 years ago by vdelecroix

• Description modified (diff)

### comment:4 follow-up: ↓ 6 Changed 7 years ago by brett

In case "Unkown" did you mean that Sage does not know how to do it (as currently implemented) but possibly Mathematics knows the answer? "Unknown" should ideally mean that mathematics does not know the answer but Sage development is likely to lag behind mathematics and I don't want users to think that if Sage does not know the answer that this implies that mathematics does not know the answer. I think this is all what you meant but the word "nor" confused me.

The Bruck-Ryser-Chowla Theorem (from Theorem 2.13 in Stinson's Book) shows that if $n \equiv 1,2 \bmod 4$ and there exists a prime $p \equiv 3 \bmod 4$ such that the largest power of $p$ that divides $n$ is odd. Then a TD$(n+1,n)$ does not exist.

A reference for the non existence of TD$(11,10)$ is

@article {MR1018454,
AUTHOR = {Lam, C. W. H. and Thiel, L. and Swiercz, S.},
TITLE = {The nonexistence of finite projective planes of order {$10$}},
JOURNAL = {Canad. J. Math.},
FJOURNAL = {Canadian Journal of Mathematics. Journal Canadien de Math\'ematiques},
VOLUME = {41},
YEAR = {1989},
NUMBER = {6},
PAGES = {1117--1123},
ISSN = {0008-414X},
CODEN = {CJMAAB},
MRCLASS = {51E15 (51-04)},
MRNUMBER = {1018454 (90j:51008)},
MRREVIEWER = {J. C. Fisher},
DOI = {10.4153/CJM-1989-049-4},
}


For the results I mentioned about sufficiently high $k$ implying TD$(n+1,n)$ exists, I will have to dig up the papers by Metz and review them to remember exactly how this works.

Last edited 7 years ago by ncohen (previous) (diff)

### comment:5 Changed 7 years ago by brett

Why did that BibTeX reference format so badly?

### comment:6 in reply to: ↑ 4 ; follow-up: ↓ 7 Changed 7 years ago by ncohen

Yo !

In case "Unkown" did you mean that Sage does not know how to do it (as currently implemented) but possibly Mathematics knows the answer?

The parameter will be called "existence". When you type designs.transversal_design(k,n,existence=True)

it will answer 1) True meaning "Sage can build it" 2) Unknown meaning "Sage can't build it, but it may exist" 3) False meaning "it does not exist"

Thus there is nothing there to mean "Mathematics knows it exists but Sage does not know how to build it". THouuuuuuugh if you know of something like that, the only responsible way to solve the problem is to implement the mathematical result :-P

"Unknown" should ideally mean that mathematics does not know the answer but Sage development is likely to lag behind mathematics and I don't want users to think that if Sage does not know the answer that this implies that mathematics does not know the answer. I think this is all what you meant but the word "nor" confused me.

Well, the word "unknown" means "Sage has no idea" ^^;

For the results I mentioned about sufficiently high $k$ implying TD$(n+1,n)$ exists, I will have to dig up the papers by Metz and review them to remember exactly how this works.

Okayyyyyyyyy !

Why did that BibTeX reference format so badly?

Because some characters that you used are interpreted as typographic instructions. Wrap what you want to say with {{{ and }}} to avoid that. I edited your comment above, so it looks fine now ;-)

Nathann

### comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 7 years ago by brett

Yo !

In case "Unkown" did you mean that Sage does not know how to do it (as currently implemented) but possibly Mathematics knows the answer?

The parameter will be called "existence". When you type designs.transversal_design(k,n,existence=True)

it will answer 1) True meaning "Sage can build it" 2) Unknown meaning "Sage can't build it, but it may exist" 3) False meaning "it does not exist"

Perfect, this is what I thought, but "nor" threw me off

Why did that BibTeX reference format so badly?

Because some characters that you used are interpreted as typographic instructions. Wrap what you want to say with {{{ and }}} to avoid that. I edited your comment above, so it looks fine now ;-)

thanks

You put it in some kind of box, a "code block"?

### comment:8 in reply to: ↑ 7 Changed 7 years ago by ncohen

You put it in some kind of box, a "code block"?

Yep !

### comment:9 Changed 7 years ago by ncohen

• Branch set to public/16272
• Status changed from new to needs_review

Here it is ! I implemented the same thing so many times that even I am trying to see that unifying it all may help... :-P

Nathann

### comment:10 Changed 7 years ago by ncohen

• Status changed from needs_review to needs_work

### comment:11 Changed 7 years ago by git

• Commit set to d678326f182432584dd0aaf99308038aa70961e5

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

 ​3fc79ba trac #15310: Merged into 6.2.rc1 ​054d2a2 trac #16227: Product construction of Transversal Designs ​a512ab1 trac #16227: Merged with updated #15310 ​e62fae8 corrected doctests + new doctests ​4d6e964 trac #16227: Replace exception with booleans in the doctests ​a46446f trac #16231: Equivalence between OA/TD/MOLS ​a9dce70 trac #16231: Merged with updated #16227 ​8d8b928 more documentation to orthogonal_arrays.py ​8257178 remove MOLS construction for prime powers + doc ​d678326 trac #16272: Replacing availability by existence and forwarding the results between design constructors

### comment:12 follow-up: ↓ 13 Changed 7 years ago by vdelecroix

Hi,

I would like to have a verbose or message option:

sage: designs.transversal_design(7,6,existence=True)
False
sage: designs.transversal_design(7,6,existence=True,message=True)
(False, "By the Bruck-Ryser-Chowla theorem, no projective plane of order 6 exists")


But it looks like we are implementing ourself the propagation of errors... What do you think?

Vincent

### comment:13 in reply to: ↑ 12 Changed 7 years ago by ncohen

Yo !

But it looks like we are implementing ourself the propagation of errors... What do you think?

HMmm... Well, to me this is similar to the "construction path" feature. You know, the feature to tell you how to prove formally that a design exists with theorems ? There you want a "formal proof" that it cannot be done.

The thing is that right now we have no recursive proof of non-existence... But I still believe that it is too early to implement stuff like that.

Nathann

### comment:14 Changed 7 years ago by ncohen

• Component changed from PLEASE CHANGE to combinatorics

### comment:15 follow-up: ↓ 19 Changed 7 years ago by vdelecroix

Hello,

Here it comes!

Ok. I mainly did two things

• implement my own projective_space_as_OA inside orthogonal_arrays.py. This might sound weird but the current implementation of ProjectiveSpaceDesign is slow and scarry me (I plan to clean it soon in a ticket).
• implement a beautiful TD_existence (I do not like the name, so I aks for suggestions here):
sage: from sage.combinat.designs.orthogonal_arrays import TD_existence
sage: TD_existence(6)
TD(k,6)
exist            for k in [ 2, 3]
unknown to exist for k in [ 4, 6]
not exist        for k in [ 7,+infini]


I also correct some of the errors raised to get mainly EmptySetError and NotImplementedError.

Vincent

### comment:16 Changed 7 years ago by git

• Commit changed from d678326f182432584dd0aaf99308038aa70961e5 to ba058dd515c38084b4cdeacf530eda7b4563d211

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

 ​8403829 16272: more functionalities ​ba058dd more doctests

### comment:17 Changed 7 years ago by vdelecroix

• Status changed from needs_work to needs_review

As I do not have the reference for the third point mentioned in the ticket, I did not do it...

Vincent

### comment:18 Changed 7 years ago by vdelecroix

• Description modified (diff)

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

Yo !

• implement my own projective_space_as_OA inside orthogonal_arrays.py. This might sound weird but the current implementation of ProjectiveSpaceDesign is slow and scarry me (I plan to clean it soon in a ticket).

THis thing has *NOTHING* to do here. Use the current ProjectivePlaneDesigns, and write another ticket to change the code if you don't like it. I agree that it is slow, but implementing the function TWICE (one that is slow, one that is hidden in an unrelated module) is not a way to solve it.

• implement a beautiful TD_existence (I do not like the name, so I aks for suggestions here):

Beautiful ? Is that a joke ? A function that PRINTS text ?

Get this thing out of here until a proper interface design is found. A function like that should RETURN values, not print stuff. Plus we have almost no negative result to advertise for the moment. Plus how the hell is an user going to find that ? We write Sage code here, not our own scripts !!!

If such a function is to ever be implemented, it will return a pair "lower bound, upper bound". The only interesting data in your three lines are TWO NUMBERS, and wrapping them in intervals for nothing, adding a 2 there and an infinity there looks fancy, but once more we work on a library here, not on your own code.

I also correct some of the errors raised to get mainly EmptySetError and NotImplementedError.

I will look at that when the problems above are solved....

Nathann

### comment:20 Changed 7 years ago by ncohen

By the way... When we will have merged all constructions, there is a doctest that I would like to include in the doc, and it may partly do what you built this "TD_existence" thing for.

This is the table that I am trying to copy : http://books.google.fr/books?id=S9FA9rq1BgoC&lpg=PA175&ots=Do3xYTkMox&dq=%22mols%22%20of%20side%20%3C%2010000&pg=PA176#v=onepage&q=%22mols%22%20of%20side%20%3C%2010000&f=false

It is somehow "the reference". It gives you the maximum number of MOLS that are known to exist for a given order.

aaaaaaaand I have had the following code for a while to see where I was going:

print "      ",
for i in range(20):
print str(i)+" "*(4-len(str(i))),
print ""
print " "*4+"_"*20*5+"__"

for i in range(20*10):
if i%20==0:
print ""
print str(i)+" "*(4-len(str(i)))+"| ",
if i < 2:
k = 1
else:
for k in range(i+1):
if not designs.mutually_orthogonal_latin_squares(i,k,availability=True):
break
k = str(k-1)
print k+" "*(4-len(k)),


This function is not meant to be called by users of course, I thought I would just make it a private function and call it just once, in a "# long" doctest. Here is what it produces, with #16241 (and all its dependencies) applied.

       0    1    2    3    4    5    6    7    8    9    10   11   12   13   14   15   16   17   18   19
______________________________________________________________________________________________________

0   |  0    0    1    2    3    4    1    6    7    8    2    10   4    12   4    4    15   16   3    18
20  |  4    5    3    22   7    24   4    26   5    28   4    30   31   5    4    5    8    36   4    5
40  |  7    40   5    42   5    6    4    46   8    48   6    5    5    52   5    6    7    3    2    58
60  |  5    60   5    6    63   4    2    66   4    4    6    70   7    72   3    7    3    6    3    78
80  |  9    80   8    82   6    6    6    3    7    88   3    6    3    4    3    6    7    96   6    8
100 |  8    100  6    102  7    4    4    106  4    108  4    6    7    112  3    7    4    8    3    6


It will be a nice way to advertise what Sage can do for TD/OA/MOLS :-P

Nathann

### comment:21 Changed 7 years ago by git

• Commit changed from ba058dd515c38084b4cdeacf530eda7b4563d211 to bcf917589a03c3e71a800a51d181ed24a96834f9

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

 ​bcf9175 16272: reviewer comment 1

### comment:22 follow-up: ↓ 23 Changed 7 years ago by vdelecroix

Hi Nathann,

You are right. I moved TD_existence as a doctest in transversal_design and now projective_plane_as_OA belongs to the module block_design.

The function projective_plane_as_OA is not similar to ProjectivePlaneDesign as it returns an OA (moreover, possibly not a complete OA(n+1,n,2)). To write it, I just pick the code from what was in OA and put it in an independent function. To my mind, we should keep the ProjectivePlaneDesign function as it was before.

Is it better like that?

Vincent

### comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 7 years ago by ncohen

Yo !

You are right. I moved TD_existence as a doctest in transversal_design and now projective_plane_as_OA belongs to the module block_design.

Okay, I really think that you are wrongly assuming that it is "always cool" to add tests, but I always check the long tests when I work on a file and adding "a minute there, a minute there" really is painful.

Let's talk about the code : you screamed when I removed the feature "give me the largest k possible" from the MOLS. It is now time to add it. To add the feature in all 3 constructors, because it costs absolutely nothing

def OA(...):
if k is None:
all(OA(k+1,n,existence=True) for k in range(2,n+4))
return k


And for all others

def TD(...):
if k is None:
k = OA(None, n,existence = True)


Then your doctest will be lighter and we will have the feature back. I can do it if you don't want to.

Also : would you mind not constructing the design for values of k which are smaller than the best possible ? Really, please don't make doctests too long. It takes times when you work on code, and you really over-check stuff. Remember that all results are checked before being returned anyway, so really this is a waste of (developer) time.

The function projective_plane_as_OA is not similar to ProjectivePlaneDesign as it returns an OA (moreover, possibly not a complete OA(n+1,n,2)). To write it, I just pick the code from what was in OA and put it in an independent function. To my mind, we should keep the ProjectivePlaneDesign function as it was before.

If you need to rewrite a different version of ProjectivePlaneDesign becaause it is to slow, is it safe to assume that ProjectivePlaneDesign should be changed ?

It does not make any sense to implement the same thing twice, Vincent, really. Of course if you have to call the ProjectivePlaneDesign you may have some conversion to do, but is that really a problem ? We must not implement this thing twice, really ....

Nathann

### comment:24 in reply to: ↑ 23 ; follow-ups: ↓ 25 ↓ 28 Changed 7 years ago by vdelecroix

Yo !

You are right. I moved TD_existence as a doctest in transversal_design and now projective_plane_as_OA belongs to the module block_design.

Okay, I really think that you are wrongly assuming that it is "always cool" to add tests, but I always check the long tests when I work on a file and adding "a minute there, a minute there" really is painful.

Let's talk about the code : you screamed when I removed the feature "give me the largest k possible" from the MOLS. It is now time to add it. To add the feature in all 3 constructors, because it costs absolutely nothing

def OA(...):
if k is None:
all(OA(k+1,n,existence=True) for k in range(2,n+4))
return k


And for all others

def TD(...):
if k is None:
k = OA(None, n,existence = True)


Then your doctest will be lighter and we will have the feature back. I can do it if you don't want to.

Also : would you mind not constructing the design for values of k which are smaller than the best possible ? Really, please don't make doctests too long. It takes times when you work on code, and you really over-check stuff. Remember that all results are checked before being returned anyway, so really this is a waste of (developer) time.

Sounds good to me. I will do it and simplify the doctests accordingly.

The function projective_plane_as_OA is not similar to ProjectivePlaneDesign as it returns an OA (moreover, possibly not a complete OA(n+1,n,2)). To write it, I just pick the code from what was in OA and put it in an independent function. To my mind, we should keep the ProjectivePlaneDesign function as it was before.

If you need to rewrite a different version of ProjectivePlaneDesign because it is too slow, is it safe to assume that ProjectivePlaneDesign should be changed ?

It is not only about conversion. ProjectivePlaneDesign is buggy: there is crazy argument "type" which makes specification a bit more complicated:

• if type is None: do what you know is the best and return a projective plane
• if type is something: do whatever the construction tells you and return the corresponding projective plane

Now if you incorporate the who_asked feature. It becomes crazy! I feel like type and who_asked are rather incompatible. To my mind, we should simply remove ProjectivePlaneDesign and have different functions (which raise only ValueError on bad entries) for each kind of projective planes we know about.

It does not make any sense to implement the same thing twice, Vincent, really. Of course if you have to call the ProjectivePlaneDesign you may have some conversion to do, but is that really a problem ? We must not implement this thing twice, really ....

What? I only moved your code from orthogonal_array to projective_plane_as_OA? Just kidding :-P

### comment:25 in reply to: ↑ 24 Changed 7 years ago by vdelecroix

Yo !

You are right. I moved TD_existence as a doctest in transversal_design and now projective_plane_as_OA belongs to the module block_design.

Okay, I really think that you are wrongly assuming that it is "always cool" to add tests, but I always check the long tests when I work on a file and adding "a minute there, a minute there" really is painful.

Let's talk about the code : you screamed when I removed the feature "give me the largest k possible" from the MOLS. It is now time to add it. To add the feature in all 3 constructors, because it costs absolutely nothing

def OA(...):
if k is None:
all(OA(k+1,n,existence=True) for k in range(2,n+4))
return k


And for all others

def TD(...):
if k is None:
k = OA(None, n,existence = True)


Then your doctest will be lighter and we will have the feature back. I can do it if you don't want to.

Also : would you mind not constructing the design for values of k which are smaller than the best possible ? Really, please don't make doctests too long. It takes times when you work on code, and you really over-check stuff. Remember that all results are checked before being returned anyway, so really this is a waste of (developer) time.

Sounds good to me. I will do it and simplify the doctests accordingly.

Arrrrrrggggghhhhhhh. The k comes before the n. So there is no way to make k optional without making n optional... do you feel like having something like

def transversal_design(k,n=None):
if n is None:
n = k
k = best_possible(n)
...


### comment:26 Changed 7 years ago by git

• Commit changed from bcf917589a03c3e71a800a51d181ed24a96834f9 to ba89d70cda40900f7410574f2d0c3f987f5f47b8

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

 ​ba89d70 shorten doctest in orthogonal_arrays.py

### comment:27 Changed 7 years ago by vdelecroix

Now long doctest takes 25 secs on orthogonal_arrays.py, must be good enough.

### comment:28 in reply to: ↑ 24 Changed 7 years ago by ncohen

It is not only about conversion. ProjectivePlaneDesign is buggy: there is crazy argument "type" which makes specification a bit more complicated:

• if type is None: do what you know is the best and return a projective plane
• if type is something: do whatever the construction tells you and return the corresponding projective plane

This ticket is no place to fix that.

Now if you incorporate the who_asked feature. It becomes crazy! I feel like type and who_asked are rather incompatible. To my mind, we should simply remove ProjectivePlaneDesign and have different functions (which raise only ValueError on bad entries) for each kind of projective planes we know about.

Vincent, the "type" keyword is not even USED. Remove it if it makes it easier for you.

Nathann

### comment:29 Changed 7 years ago by vdelecroix

I can not use the current ProjectivePlaneDesign... here are the timings:

sage: %timeit designs.ProjectivePlaneDesign(9)
1 loops, best of 3: 2.63 s per loop
sage: %timeit designs.ProjectivePlaneDesign(16)  # WTF?
1 loops, best of 3: 42.3 s per loop


2 secs are reasonable but 42 secs are not and I do not want to go further. Now with the straigthforward one I used it takes less than 1 sec:

sage: from sage.combinat.designs.block_design import projective_plane_as_OA
sage: %timeit projective_plane_as_OA(9)
100 loops, best of 3: 5.95 ms per loop
sage: %timeit projective_plane_as_OA(16)
10 loops, best of 3: 39.5 ms per loop


I do not want either to rewrite the whole ProjectivePlaneDesign as it is not the purpose of that ticket. So please find a reasonable solution.

### comment:30 Changed 7 years ago by vdelecroix

• Dependencies changed from #15310, #16227 to #15310, #16227, #16281
• Status changed from needs_review to needs_work

I agreed with Nathann that the best solution is to rewrite the projective planes... so here it is: #16281

### comment:31 Changed 7 years ago by git

• Commit changed from ba89d70cda40900f7410574f2d0c3f987f5f47b8 to 8830bb57327e1ca370287ca60b33e6d1b790a2d3

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

 ​1ded990 trac #16272: report appropriate timings for the long doctest ​429d815 16281: rewrite projective planes (as 2-designs) ​3e5628a 16281: reviewer's comment 1 ​0812a73 trac #16281: Simplification ​61dc86b 16281: long comment for the construction of the projective plane ​51daa7f trac #16281: correct a doctest ​01722c4 trac #16272: merge #16281 (first part) ​8830bb5 trac #16272: merge #16281 (part 2)

### comment:32 Changed 7 years ago by vdelecroix

• Status changed from needs_work to needs_review

Hi Nathann,

Projective planes from #16281 is merged and now the code is shorter/cleaner.

There is no real check that a projective plane is valid as the ouptut of the function projective_plane... but that is not a real problem.

Vincent

### comment:33 Changed 7 years ago by ncohen

Well, not only we don't care if some step of the total computation is not checked, but none of them should be checked except the last one. We should never call design constructors from inside of the library without deacivating the checks othewise we really waste time.

Okay... Now I am going to play with git to see what exactly is being implemented in this ticket T_T

Nathann

### comment:34 Changed 7 years ago by ncohen

Please Vincent. In the future, do not do ANYTHING in a merge commit except fix the conflicts.

It is HELL to know what else you changed.

Nathann

### comment:35 Changed 7 years ago by ncohen

Okay, it is hell to know what exactly you did here. Can you tell me ?

For instance, what is the point of this who_asked in projective plane ?

Nathann

### comment:36 Changed 7 years ago by ncohen

Okay Vincent. Unless you know a way for me to see exactly what you implemented which is not already in the other tickets, could you :

1) GO back to the history before the merge 2) Do the merge and only the merge 3) Implement stuff

Otherwise I just can't see what you did.

Nathann

Version 0, edited 7 years ago by ncohen (next)

### comment:37 Changed 7 years ago by vdelecroix

Hi,

Before the merge, there was a who_asked argument in projective_plane_as_OA. Now it is an argument of projective_plane. In the second part of the merge I only fix some typo and doctests.

Perhaps I should go much before in history and then do the merge... let me try.

Vincent

### comment:38 Changed 7 years ago by ncohen

Oh. I found a way. Cool O_O;;

Vincent, I think there is absolutely no point to the implementation of the who_asked argument in projective plane, nor in projective plane calling OA back. No projective plane is known to exist for orders which are not prime powers, so this is going too far. We will adapt the code if this ever happens

Nathann

### comment:39 follow-up: ↓ 40 Changed 7 years ago by ncohen

It seems I still cannot see all changes. I saw that though :

+    #TODO: here there is no need to test everything as the function
+    # projective_plane answers it for you


If you want this to be removed, you need an existence flag on projective_plane.

Nathann

### comment:40 in reply to: ↑ 39 Changed 7 years ago by ncohen

If you want this to be removed, you need an existence flag on projective_plane.

Oh, we have it already. Cool. Well, you can replace it if you want, then.

Nathann

### comment:41 Changed 7 years ago by vdelecroix

Time to eat... will have a look later tonight.

Vincent

### comment:42 follow-up: ↓ 43 Changed 7 years ago by vdelecroix

It will be much cleaner if I start the merge from d678326 (your last commit)... let's go!

Vincent

### comment:43 in reply to: ↑ 42 Changed 7 years ago by ncohen

It will be much cleaner if I start the merge from d678326 (your last commit)... let's go!

+1

Nathann

### comment:44 Changed 7 years ago by git

• Commit changed from 8830bb57327e1ca370287ca60b33e6d1b790a2d3 to 5074eee1d802ef44c251252581684bf1dc1dc7d6

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

 ​e090f92 trac #16272: merge #16281 ​9a221be trac #16272: fix doctests ​5074eee trac #16272: finer doctest to test the output of transversal_design

### comment:45 Changed 7 years ago by vdelecroix

Hi Nathann,

This is a non fast forward push from your last commit. I integrated your remarks and the long doctest I created to check the output of transversal_design.

Vincent

### comment:46 Changed 7 years ago by ncohen

Thanks ! It will be easier to follow this way :-)

Nathann

### comment:47 Changed 7 years ago by vdelecroix

Note that the doc fails to build

[combinat ] /opt/sage/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays.py:docstring of sage.combinat.designs.orthogonal_arrays.find_wilson_decomposition:14: ERROR: Unexpected indentation.
[combinat ] :0: WARNING: Block quote ends without a blank line; unexpected unindent.
Error building the documentation.
Traceback (most recent call last):
File "/opt/sage/src/doc/common/builder.py", line 1477, in <module>
getattr(get_builder(name), type)()
File "/opt/sage/src/doc/common/builder.py", line 276, in _wrapper
getattr(get_builder(document), 'inventory')(*args, **kwds)
File "/opt/sage/src/doc/common/builder.py", line 487, in _wrapper
x.get(99999)
File "/opt/sage/local/lib/python/multiprocessing/pool.py", line 554, in get
raise self._value
OSError: [combinat ] /opt/sage/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays.py:docstring of sage.combinat.designs.orthogonal_arrays.find_wilson_decomposition:14: ERROR: Unexpected indentation.


### comment:48 follow-up: ↓ 51 Changed 7 years ago by ncohen

Yooooooooooo !!

I understand that you do not want to call the same function twice, but would you mind behaving as if boolean results were already cached ? It really makes the code's flow easier to follow :-/

Otherwise everything looks right... If I may ask, I think it would also be better to not have this TD = designs.transversal_design in the doctest. Keeping the full writing also helps explaining to the users that all the TD can be built from designs.transversal_design.

Nathann

Last edited 7 years ago by ncohen (previous) (diff)

### comment:49 Changed 7 years ago by ncohen

Besides it's good to go.

Nathann

### comment:50 Changed 7 years ago by git

• Commit changed from 5074eee1d802ef44c251252581684bf1dc1dc7d6 to d81f265637099f93495dafac54e2d354be0d66d5

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

 ​d81f265 trac #16272: ultimate doctest

### comment:51 in reply to: ↑ 48 ; follow-up: ↓ 52 Changed 7 years ago by vdelecroix

Yooooooooooo !!

I understand that you do not want to call the same function twice, but would you mind behaving as if boolean results were already cached ? It really makes the code's flow easier to follow :-/

I do not care too much about calling twice the function. My main trouble was that in OA with parameters n,k=n+1,t=2 we have equivalence with projective planes. So we need to let the projective plane do what it has to do (even if the answer is Unknown). Whereas for general n,k,t=2 we might have something better but it still worth it to ask about a projective plane... If you have an idea to make it neat I let it to you.

Otherwise everything looks right... If I may ask, I think it would also be better to not have this TD = designs.transversal_design in the doctest. Keeping the full writing also helps explaining to the users that all the TD can be built from designs.transversal_design.

Cool (Note that the doc problem is fixed).

We leave the idea of having an optional k=None? Might be an independent ticket anyway.

Vincent

### comment:52 in reply to: ↑ 51 Changed 7 years ago by ncohen

I do not care too much about calling twice the function.

Cool. Then I changed the code to have a nice "if/elif" structure. In some very specific cases the projective plane function is called three times, but

1) This is only when the answer has been found, so it does not happens often per call

2) A tricky replacement is possible too, as

    elif (t == 2 and
(projective_plane(n, existence=True) or
(k == n+1 and projective_plane(n, existence=True) is False))):


is equivalent to:

    elif (t == 2 and
(projective_plane(n, existence=True) in (True,k<n+1))):


This being said the code of these things compared to the cost of building the actual array ...

We leave the idea of having an optional k=None? Might be an independent ticket anyway.

Yep, let's close this one and get #16277 in. I will merge it in a second.

Nathann

### comment:53 Changed 7 years ago by git

• Commit changed from d81f265637099f93495dafac54e2d354be0d66d5 to 47798d2d54221c0bd0ce6aa75d1900496a091e2f

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

 ​47798d2 trac #16272: simplifying the structure of orthogonal_array

### comment:54 follow-up: ↓ 55 Changed 7 years ago by vdelecroix

Hi,

This is what I did not want to do... if k=n+1 and projective_plane answers Unknown you are going to try other constructions. In comment:38 you said that it was useless... But anyway, it does not cost too much and if by chance we got one we would be happy!

Who is the author? who is the reviewer? who sets to positive review?

Vincent

### comment:55 in reply to: ↑ 54 Changed 7 years ago by ncohen

Yo !

This is what I did not want to do... if k=n+1 and projective_plane answers Unknown you are going to try other constructions. In comment:38 you said that it was useless...

Oh right. But that was to avoid having a who_asks parameter in projective_plane. But anyway, it does not cost too much and if by chance we got one we would be happy! Ahahaha. Yeah. Perhaps somebody overlooked a very simple case of recursive decomposition ;-)

Who is the author? who is the reviewer? who sets to positive review?

Well, let's do half/half. Positive review with both reviewers and both authors. By the way I just finished the patch for k=None and I added the MOLS table from my dreams. The last dependency of the constructions will also be reviewed in a second ;-)

Nathann

### comment:56 Changed 7 years ago by ncohen

• Authors changed from Vincent Delecroix to Nathann Cohen, Vincent Delecroix
• Reviewers set to Nathann Cohen, Vincent Delecroix
• Status changed from needs_review to positive_review

### comment:57 Changed 7 years ago by git

• Commit changed from 47798d2d54221c0bd0ce6aa75d1900496a091e2f to d34b012758319ebcea5c0a0e17e6481ad5ded481
• 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 ​d34b012 trac #16272: Merged with updated #16227

### comment:58 Changed 7 years ago by ncohen

• Status changed from needs_review to positive_review

Still merging ...

Nathann

### comment:59 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:60 Changed 7 years ago by vbraun

• Status changed from positive_review to needs_work

Merge conflict

### comment:61 Changed 7 years ago by git

• Commit changed from d34b012758319ebcea5c0a0e17e6481ad5ded481 to c127a6d52c126483966dbc5faa6436d699a0d4a4

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

 ​d051cf4 ValueError -> EmptySetError in latin_squares ​9e5e94f trac #16231: Merged with updated #16227 ​c127a6d trac #16272: Merged with updated #16231

### comment:62 Changed 7 years ago by ncohen

• Dependencies changed from #15310, #16227, #16281 to #16231
• Status changed from needs_work to positive_review

### comment:63 Changed 7 years ago by git

• Commit changed from c127a6d52c126483966dbc5faa6436d699a0d4a4 to e2749b3db4cd78fc5e9ea8219d4cdf895b283465
• 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:

 ​e2749b3 trac #16272: Merged with 6.3.beta0

### comment:64 Changed 7 years ago by ncohen

• Status changed from needs_review to positive_review

### comment:65 Changed 7 years ago by vbraun

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