Opened 4 years ago

Closed 4 years ago

Last modified 22 months ago

#17034 closed enhancement (fixed)

New user interface for orthogonal arrays and a .explain_construction method

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.4
Component: combinatorial designs Keywords:
Cc: vdelecroix Merged in:
Authors: Nathann Cohen Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: b9516b1 (Commits) Commit:
Dependencies: Stopgaps:

Description

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

Change History (33)

comment:1 Changed 4 years ago by ncohen

  • Branch set to u/ncohen/17034
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to 60911298a7c3bd33e4f9b7e73de61acc70dfd445

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

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

comment:3 Changed 4 years ago by vdelecroix

Hey,

Cool! I will add a look ASAP.

Vincent

comment:4 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

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

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

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

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

comment:8 Changed 4 years ago by git

  • Commit changed from 60911298a7c3bd33e4f9b7e73de61acc70dfd445 to e37125b68b03058d52b44404b400af9865886ff1

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

799845atrac #17034: New user interface for orthogonal arrays and a .explain_construction method
e37125btrac #17034: Bugfix

comment:9 follow-up: Changed 4 years ago by 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

comment:10 follow-up: Changed 4 years ago by 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

comment:11 in reply to: ↑ 10 Changed 4 years ago by vdelecroix

Replying to jdemeyer:

Also, there is this funny comment

Note to self: when

This funny comment is removed in my commit!

comment:12 in reply to: ↑ 9 Changed 4 years ago by 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

comment:13 Changed 4 years ago by git

  • Commit changed from e37125b68b03058d52b44404b400af9865886ff1 to 024f0b3bf6f01d2aa4c42151d0ea56ed7fd2fbcd

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

3bf5400trac #17034: documentation
024f0b3trac #17034: Reviewer's remark

comment:14 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:15 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_info

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

comment:16 follow-up: Changed 4 years ago by 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

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

comment:18 in reply to: ↑ 16 Changed 4 years ago by ncohen

  • Branch changed from u/ncohen/17034 to u/vdelecroix/17034
  • Commit changed from 024f0b3bf6f01d2aa4c42151d0ea56ed7fd2fbcd to c7eae7de7591d2f8f9c2f4de13396307ac210028
  • Status changed from needs_info to needs_review

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:

c7eae7dtrac #17034: move the user functions into OAMainFunctions
Last edited 4 years ago by ncohen (previous) (diff)

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

comment:20 follow-up: Changed 4 years ago by 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

comment:21 in reply to: ↑ 19 Changed 4 years ago by 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

comment:22 in reply to: ↑ 20 Changed 4 years ago by ncohen

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

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

comment:23 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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)

comment:24 in reply to: ↑ 23 ; follow-up: Changed 4 years ago by ncohen

  • Branch changed from u/vdelecroix/17034 to public/17034
  • Commit changed from c7eae7de7591d2f8f9c2f4de13396307ac210028 to db9bd8d6a5e227c9cbf9f918382e913bed8bfa5b
  • Status changed from needs_work to positive_review

(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:

db9bd8dtrac #17034: assert has been declared illegal

comment:25 Changed 4 years ago by git

  • Commit changed from db9bd8d6a5e227c9cbf9f918382e913bed8bfa5b to b0467933bee693c0a71a83cba53af049abb0c23c
  • Status changed from positive_review to needs_review

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

b046793trac #17034: assert has been declared illegal

comment:26 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:27 in reply to: ↑ 24 Changed 4 years ago by 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.

comment:28 Changed 4 years ago by vbraun

  • Branch changed from public/17034 to b0467933bee693c0a71a83cba53af049abb0c23c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:29 Changed 4 years ago by vbraun

  • Commit b0467933bee693c0a71a83cba53af049abb0c23c deleted
  • Resolution fixed deleted
  • Status changed from closed to new
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.
**********************************************************************

comment:30 Changed 4 years ago by ncohen

  • Branch changed from b0467933bee693c0a71a83cba53af049abb0c23c to public/17034
  • Commit set to b9516b194fdbcd8ec5e17e6d251b63ef9ac3d7e8
  • Status changed from new to needs_review

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

Nathann


New commits:

799845atrac #17034: New user interface for orthogonal arrays and a .explain_construction method
e37125btrac #17034: Bugfix
3bf5400trac #17034: documentation
024f0b3trac #17034: Reviewer's remark
c7eae7dtrac #17034: move the user functions into OAMainFunctions
b046793trac #17034: assert has been declared illegal
b9516b1trac #17034: Broken doctest

comment:31 Changed 4 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:32 Changed 4 years ago by vbraun

  • Branch changed from public/17034 to b9516b194fdbcd8ec5e17e6d251b63ef9ac3d7e8
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 22 months ago by jdemeyer

  • Commit b9516b194fdbcd8ec5e17e6d251b63ef9ac3d7e8 deleted

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.

Last edited 22 months ago by jdemeyer (previous) (diff)
Note: See TracTickets for help on using tickets.