New user interface for orthogonal arrays and a .explain_construction method

[ncohen]

With this branch the users do not have access to the "designs.orthogonal_array" function which returns int/bool/OA(/string) values. It is deprecated, and replaced with "designs.orthogonal_arrays.*" function that do the same.

Instead of having 3 times the same functions for MOLS and TD, I chosed to remove the arguments in those other constructors, redirecting the users toward the OA functions. I believe that it is better in the long run, as copy/pasting everything really seems pointless.

Additionally, this branch adds a *NICE* feature:

sage: print designs.orthogonal_arrays.explain_construction(10,151)
From a projective plane of order 151
sage: print designs.orthogonal_arrays.explain_construction(10,935)
Brouwer's separable design construction with t=16,q=7,x=23 from:
    Andries E. Brouwer,
    A series of separable designs with application to pairwise orthogonal Latin squares
    Vol. 1, n. 1, pp. 39-41,
    European Journal of Combinatorics, 1980

[git]

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

​6091129

trac #17034: New user interface for orthogonal arrays and a .explain_construction method

[vdelecroix]

Hey,

Cool! I will add a look ASAP.

Vincent

[vdelecroix]

Hello,

I like much better the interface now!

In OA_build the argument resolvable is not used (and not tested).

Why do you still allow k=None in the function orthogonal_array? It would be simpler/cleaner if it was only managed by largest_available. No?

Vincent

[ncohen]

Yo !

I like much better the interface now!

Well, I have a lot of things to write when this will be reviewed. Like the functions to get all n such that there exists an OA(k,n), and stuff for incomplete orthogonal arrays too that I need for yet other applications of the Brouwer van Rees construction.

All the stuff that I cannot write when everything is done through the same function.

In OA_build the argument resolvable is not used (and not tested).

Oh. Right. That's because I had created a "build_resolvable" function at some point in the interface, and did not add it in the end.

Why do you still allow k=None in the function orthogonal_array? It would be simpler/cleaner if it was only managed by largest_available. No?

It depends on you. I hate the very principle of backward compatibility, but that's why it is there, because the guys who call designs.orthogonal_array and will persist in doing so for a year even if it displays a warning need to be able to call it anyway, or so I am told.

When this thing will not be deprecated anymore this will only be internal code and we will be allowed to remove whatever we want whenever we want.

Nathann

[vdelecroix]

Replying to ncohen:

Why do you still allow k=None in the function orthogonal_array? It would be simpler/cleaner if it was only managed by largest_available. No?

It depends on you. I hate the very principle of backward compatibility, but that's why it is there, because the guys who call designs.orthogonal_array and will persist in doing so for a year even if it displays a warning need to be able to call it anyway, or so I am told.

When this thing will not be deprecated anymore this will only be internal code and we will be allowed to remove whatever we want whenever we want.

I see. Following the deprecation rules, the code will not be deleted before sage 6.5. And at that time we might have forget about this! What do you think about an extra deprecation there?

Vincent

[ncohen]

Yo !

I see. Following the deprecation rules, the code will not be deleted before sage 6.5. And at that time we might have forget about this! What do you think about an extra deprecation there?

I do not know what you call "extra deprecation" but I added a note near another note about that (see commit).

Nathann

[git]

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

​799845a	trac #17034: New user interface for orthogonal arrays and a .explain_construction method
​e37125b	trac #17034: Bugfix

[vdelecroix]

Hi,

I pushed a commit at u/vdelecroix/17034. With your version we add a very ugly documentation for

sage: designs.orthogonal_arrays?

I moved the examples from the function orthogonal_array to the class used for the tab completion.

I also renamed OATabThing into OAMainFunctions... it is much more explicit.

Beyond my modifications:

• why do we have designs.orthogonal_arrays.build but designs.orthogonal_arrays.exists. It should be uniform for the presence/absence of s.
• having a class OATabThing to imitate a module is really ugly... wouldn't it be better to create a new file orthogonal_arrays_catalog.py which would gather the things.
• you wrote that you would like to remove the argument existence from the function orthogonal_array... but I really do not see how we can achieve that right now without duplication!

Vincent

[jdemeyer]

Personal pet peeve: don't use assert for checking user input, use proper raise ValueError() (or whatever exception) instead.

Also, there is this funny comment

Note to self: when

[vdelecroix]

Replying to jdemeyer:

Also, there is this funny comment

Note to self: when

This funny comment is removed in my commit!

[ncohen]

Hello !

I pushed a commit at u/vdelecroix/17034. With your version we add a very ugly documentation for

sage: designs.orthogonal_arrays?

Oh, True.

I moved the examples from the function orthogonal_array to the class used for the tab completion.

Thanks

I also renamed OATabThing into OAMainFunctions... it is much more explicit.

I prefered mine. Those are not the 'main functions', they are the ones exposed to the user. The 'main function' is orthogonal_array that they don't see anymore.

Beyond my modifications:

• why do we have designs.orthogonal_arrays.build but designs.orthogonal_arrays.exists. It should be uniform for the presence/absence of s.

As we are only talking of english here, I do not agree. "Build" is an order given to Sage. As in "Build me that orthogonal array". While for 'exists' comes from "An OA(k,n) exists". In particular, you can't order something to exist. But perhaps it already exists :-P

• having a class OATabThing to imitate a module is really ugly... wouldn't it be better to create a new file orthogonal_arrays_catalog.py which would gather the things.

Well, I did that to avoid creating a new file for absolutely nothing. Otherwise it means newfile+doc module+entry in the reference manual+import the functions+import the module.

Here we have a class defined with 10 lines, I found that better.

• you wrote that you would like to remove the argument existence from the function orthogonal_array... but I really do not see how we can achieve that right now without duplication!

Oh, sorry. I will fix that in my next commit. You have no idea how many 'drafts' of this patch I wrote before deciding that it was the right way.

Nathann

[git]

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

​3bf5400	trac #17034: documentation
​024f0b3	trac #17034: Reviewer's remark

[vdelecroix]

Hello,

Does the function largest_available_k, explain_construction, OA_build, OA_exists and OA_is_available are intended to be used by programmers? I guess that it is not. So Why not defining them directly as staticmethod in OATabThing (or OAMainFunctions that you dislike)... and about the name, why not OAPublicFunctions? These are one line functions and it would cost nothing to make them not call each other.

Vincent

[vdelecroix]

For the previous remark, see the commit at u/vdelecroix/17034.

What about

sage: designs.orthogonal_arrays.is_available(1,2)
Traceback (most recent call last):
...
ValueError: undefined for k<t 
sage: from sage.combinat.designs.orthogonal_arrays import is_orthogonal_array
sage: is_orthogonal_array([[1]]*4, 1,2,2)
True

Vincent

[ncohen]

Yo !

Does the function largest_available_k, explain_construction, OA_build, OA_exists and OA_is_available are intended to be used by programmers? I guess that it is not. So Why not defining them directly as staticmethod in OATabThing (or OAMainFunctions that you dislike)... and about the name, why not OAPublicFunctions?

I really don't care about any of those points. Do it if you like, or let it stay like that.

This being said, I plan on adding new functions to "list all 'k' such that there exists an OA(k,n)", or similar functions for incomplete orthogonal arrays, and those functions may be used by both developpers and users.

These are one line functions and it would cost nothing to make them not call each other.

Why is it a problem if they call each other ?

Nathann

[ncohen]

For the previous remark, see the commit at u/vdelecroix/17034.

Oh. You already implemented it. Well let's change the branch then.

What about

sage: designs.orthogonal_arrays.is_available(1,2)
Traceback (most recent call last):
...
ValueError: undefined for k<t 
sage: from sage.combinat.designs.orthogonal_arrays import is_orthogonal_array
sage: is_orthogonal_array([[1]]*4, 1,2,2)
True

I believe that we should return a matrix filled with 0 when k<t.

Nathann

---

New commits:

​c7eae7d

trac #17034: move the user functions into OAMainFunctions

[vdelecroix]

Replying to ncohen:

This being said, I plan on adding new functions to "list all 'k' such that there exists an OA(k,n)", or similar functions for incomplete orthogonal arrays, and those functions may be used by both developpers and users.

You meant list all n I guess. Yes, that would be cool!

These are one line functions and it would cost nothing to make them not call each other.

Why is it a problem if they call each other ?

Function calls cost. I do not like using OA_is_available rather than orthogonal_array directly.

Vincent

[vdelecroix]

Hey,

I am happy with the current version. The weird case of k < t does not belong to that ticket. As I did the last commit I let you put to positive review if it deserves.

Vincent

[ncohen]

Yo !

You meant list all n I guess. Yes, that would be cool!

Oops, right.

Function calls cost. I do not like using OA_is_available rather than orthogonal_array directly.

Ahahah. True, but if the caching mechanism taught us anything it is that the actual functions do not matter much. Only the caching does :-P

Nathann

[ncohen]

I am happy with the current version. The weird case of k < t does not belong to that ticket. As I did the last commit I let you put to positive review if it deserves.

Right ! positive_review, then. If you feel like reviewing some actual constructions today, the Brouwer-van Rees has been updated with the example you wanted :-P

Thaaaaaaaaaaaaaaanks !

Nathann

[jdemeyer]

This really should be a ValueError:

sage: designs.orthogonal_arrays.largest_available_k(-1)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-1-67d3fe8bb1f0> in <module>()
----> 1 designs.orthogonal_arrays.largest_available_k(-Integer(1))

/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays.pyc in largest_available_k(n, t)
    934     """
    935     from block_design import projective_plane
--> 936     assert n>=0, "n(={}) was expected to be >=0".format(n)
    937     assert t>=0, "t(={}) was expected to be >=0".format(t)
    938     if n == 0 or n == 1:

AssertionError: n(=-1) was expected to be >=0

(use assert when you know that a condition must be true, not to check whether a condition is true)

[ncohen]

(use assert when you know that a condition must be true, not to check whether a condition is true)

Well, technically that's an 'assert' then. We added that when we noticed that some of our code was calling this function with negative values, while we *KNEW* that it had to be >=0

Nathann

---

New commits:

​db9bd8d

trac #17034: assert has been declared illegal

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

​b046793

trac #17034: assert has been declared illegal

[jdemeyer]

Replying to ncohen:

We added that when we noticed that some of our code was calling this function with negative values, while we *KNEW* that it had to be >=0

The usage of assert would indeed have been correct in the context of "some of our code calling this function". However, since this is a public function which can also be called directly, the use of an assert is wrong.

[vbraun]

sage -t --long --warn-long 59.4 src/sage/graphs/generators/intersection.py
**********************************************************************
File "src/sage/graphs/generators/intersection.py", line 418, in sage.graphs.generators.intersection.OrthogonalArrayBlockGraph
Failed example:
    OAa = designs.orthogonal_array(k,n)
Expected nothing
Got:
    doctest:1: DeprecationWarning: This function will soon be removed. Use the designs.orthogonal_arrays.* functions instead
    See http://trac.sagemath.org/17034 for details.
**********************************************************************

[ncohen]

Sorry about that. I had forgotten that the graph code called combinatorial designs.

Nathann

---

New commits:

​799845a	trac #17034: New user interface for orthogonal arrays and a .explain_construction method
​e37125b	trac #17034: Bugfix
​3bf5400	trac #17034: documentation
​024f0b3	trac #17034: Reviewer's remark
​c7eae7d	trac #17034: move the user functions into OAMainFunctions
​b046793	trac #17034: assert has been declared illegal
​b9516b1	trac #17034: Broken doctest

[jdemeyer]

In #22796, I'm removing some deprecation introduced by this ticket.

This ticket added this comment:

# When this deprecated function is removed, remove the handling of k=None in the
# function orthogonal_arrays.orthogonal_array()

but the argument k is None for orthogonal_array is still used in some doctests in src/sage/combinat/designs/orthogonal_arrays.py. So I will not do what the comment asks me to do.

In any case, the handling of k is None should be deprecated first before it can be removed.