Opened 5 years ago

Closed 5 years ago

#16272 closed enhancement (fixed)

redesign transversal designs

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

Description (last modified by vdelecroix)

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

Change History (65)

comment:1 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:2 Changed 5 years ago by vdelecroix

  • Dependencies set to #15310, #16227

comment:3 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:4 follow-up: Changed 5 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 5 years ago by ncohen (previous) (diff)

comment:5 Changed 5 years ago by brett

Why did that BibTeX reference format so badly?

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 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: Changed 5 years ago by brett

Replying to 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"

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

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

Yep !

comment:9 Changed 5 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 5 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:11 Changed 5 years ago by git

  • Commit set to d678326f182432584dd0aaf99308038aa70961e5

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

3fc79batrac #15310: Merged into 6.2.rc1
054d2a2trac #16227: Product construction of Transversal Designs
a512ab1trac #16227: Merged with updated #15310
e62fae8corrected doctests + new doctests
4d6e964trac #16227: Replace exception with booleans in the doctests
a46446ftrac #16231: Equivalence between OA/TD/MOLS
a9dce70trac #16231: Merged with updated #16227
8d8b928more documentation to orthogonal_arrays.py
8257178remove MOLS construction for prime powers + doc
d678326trac #16272: Replacing availability by existence and forwarding the results between design constructors

comment:12 follow-up: Changed 5 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 5 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 5 years ago by ncohen

  • Component changed from PLEASE CHANGE to combinatorics

comment:15 follow-up: Changed 5 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 5 years ago by git

  • Commit changed from d678326f182432584dd0aaf99308038aa70961e5 to ba058dd515c38084b4cdeacf530eda7b4563d211

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

840382916272: more functionalities
ba058ddmore doctests

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

  • Description modified (diff)

comment:19 in reply to: ↑ 15 Changed 5 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 5 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 5 years ago by git

  • Commit changed from ba058dd515c38084b4cdeacf530eda7b4563d211 to bcf917589a03c3e71a800a51d181ed24a96834f9

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

bcf917516272: reviewer comment 1

comment:22 follow-up: Changed 5 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: Changed 5 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: Changed 5 years ago by vdelecroix

Replying to 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.

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

Replying to vdelecroix:

Replying to 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.

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

  • Commit changed from bcf917589a03c3e71a800a51d181ed24a96834f9 to ba89d70cda40900f7410574f2d0c3f987f5f47b8

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

ba89d70shorten doctest in orthogonal_arrays.py

comment:27 Changed 5 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 5 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 5 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 5 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 5 years ago by git

  • Commit changed from ba89d70cda40900f7410574f2d0c3f987f5f47b8 to 8830bb57327e1ca370287ca60b33e6d1b790a2d3

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

1ded990trac #16272: report appropriate timings for the long doctest
429d81516281: rewrite projective planes (as 2-designs)
3e5628a16281: reviewer's comment 1
0812a73trac #16281: Simplification
61dc86b16281: long comment for the construction of the projective plane
51daa7ftrac #16281: correct a doctest
01722c4trac #16272: merge #16281 (first part)
8830bb5trac #16272: merge #16281 (part 2)

comment:32 Changed 5 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 5 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 5 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 5 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 5 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

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

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

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

Vincent

comment:42 follow-up: Changed 5 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 5 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 5 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:

e090f92trac #16272: merge #16281
9a221betrac #16272: fix doctests
5074eeetrac #16272: finer doctest to test the output of transversal_design

comment:45 Changed 5 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 5 years ago by ncohen

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

Nathann

comment:47 Changed 5 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: Changed 5 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 5 years ago by ncohen (previous) (diff)

comment:49 Changed 5 years ago by ncohen

Besides it's good to go.

Nathann

comment:50 Changed 5 years ago by git

  • Commit changed from 5074eee1d802ef44c251252581684bf1dc1dc7d6 to d81f265637099f93495dafac54e2d354be0d66d5

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

d81f265trac #16272: ultimate doctest

comment:51 in reply to: ↑ 48 ; follow-up: Changed 5 years ago by vdelecroix

Replying to 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 :-/

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

  • Commit changed from d81f265637099f93495dafac54e2d354be0d66d5 to 47798d2d54221c0bd0ce6aa75d1900496a091e2f

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

47798d2trac #16272: simplifying the structure of orthogonal_array

comment:54 follow-up: Changed 5 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 5 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 5 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 5 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:

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

comment:58 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Still merging ...

Nathann

comment:59 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:60 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:61 Changed 5 years ago by git

  • Commit changed from d34b012758319ebcea5c0a0e17e6481ad5ded481 to c127a6d52c126483966dbc5faa6436d699a0d4a4

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

d051cf4ValueError -> EmptySetError in latin_squares
9e5e94ftrac #16231: Merged with updated #16227
c127a6dtrac #16272: Merged with updated #16231

comment:62 Changed 5 years ago by ncohen

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

comment:63 Changed 5 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:

e2749b3trac #16272: Merged with 6.3.beta0

comment:64 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:65 Changed 5 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.