New user interface for orthogonal arrays and a .explain_construction method

[nathanncohen]

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

CC: @videlec

Component: combinatorial designs

Author: Nathann Cohen

Branch: b9516b1

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/17034

[nathanncohen]

Branch: u/ncohen/17034

[sagetrac-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

[sagetrac-git]

Commit: 6091129

[videlec]

comment:3

Hey,

Cool! I will add a look ASAP.

Vincent

[videlec]

comment:4

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

[nathanncohen]

comment:5

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

[videlec]

comment:6

Replying to @nathanncohen:

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

[nathanncohen]

comment:7

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

[sagetrac-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

[sagetrac-git]

Changed commit from 6091129 to e37125b

[videlec]

comment:9

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]

comment:10

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

[videlec]

comment:11

Replying to @jdemeyer:

Also, there is this funny comment

Note to self: when

This funny comment is removed in my commit!

[nathanncohen]

comment:12

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

[sagetrac-git]

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

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

[sagetrac-git]

Changed commit from e37125b to 024f0b3

[videlec]

comment:15

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

[videlec]

comment:16

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

[nathanncohen]

comment:17

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

[nathanncohen]

comment:24

(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

[sagetrac-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

[sagetrac-git]

Changed commit from db9bd8d to b046793

[jdemeyer]

comment:27

Replying to @nathanncohen:

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]

Changed branch from public/17034 to b046793

[vbraun]

comment:29

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.
**********************************************************************

[vbraun]

Changed commit from b046793 to none

[nathanncohen]

Commit: b9516b1

[nathanncohen]

comment:30

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

[nathanncohen]

Changed branch from b046793 to public/17034

[vbraun]

Changed branch from public/17034 to b9516b1

[jdemeyer]

Changed commit from b9516b1 to none

[jdemeyer]

comment:33

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.