#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 6 years ago by
- Branch set to u/ncohen/17034
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit set to 60911298a7c3bd33e4f9b7e73de61acc70dfd445
comment:3 Changed 6 years ago by
Hey,
Cool! I will add a look ASAP.
Vincent
comment:4 follow-up: ↓ 5 Changed 6 years ago by
- 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: ↓ 6 Changed 6 years ago by
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 argumentresolvable
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 functionorthogonal_array
? It would be simpler/cleaner if it was only managed bylargest_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: ↓ 7 Changed 6 years ago by
Replying to ncohen:
Why do you still allow
k=None
in the functionorthogonal_array
? It would be simpler/cleaner if it was only managed bylargest_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 6 years ago by
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 6 years ago by
- Commit changed from 60911298a7c3bd33e4f9b7e73de61acc70dfd445 to e37125b68b03058d52b44404b400af9865886ff1
comment:9 follow-up: ↓ 12 Changed 6 years ago by
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
butdesigns.orthogonal_arrays.exists
. It should be uniform for the presence/absence ofs
. - having a class
OATabThing
to imitate a module is really ugly... wouldn't it be better to create a new fileorthogonal_arrays_catalog.py
which would gather the things. - you wrote that you would like to remove the argument
existence
from the functionorthogonal_array
... but I really do not see how we can achieve that right now without duplication!
Vincent
comment:10 follow-up: ↓ 11 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
Hello !
I pushed a commit at
u/vdelecroix/17034
. With your version we add a very ugly documentation forsage: 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
intoOAMainFunctions
... 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
butdesigns.orthogonal_arrays.exists
. It should be uniform for the presence/absence ofs
.
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 fileorthogonal_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 functionorthogonal_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 6 years ago by
- Commit changed from e37125b68b03058d52b44404b400af9865886ff1 to 024f0b3bf6f01d2aa4c42151d0ea56ed7fd2fbcd
comment:14 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:15 follow-up: ↓ 17 Changed 6 years ago by
- 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: ↓ 18 Changed 6 years ago by
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: ↓ 19 Changed 6 years ago by
Yo !
Does the function
largest_available_k
,explain_construction
,OA_build
,OA_exists
andOA_is_available
are intended to be used by programmers? I guess that it is not. So Why not defining them directly as staticmethod inOATabThing
(orOAMainFunctions
that you dislike)... and about the name, why notOAPublicFunctions
?
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 6 years ago by
- 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:
c7eae7d | trac #17034: move the user functions into OAMainFunctions
|
comment:19 in reply to: ↑ 17 ; follow-up: ↓ 21 Changed 6 years ago by
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: ↓ 22 Changed 6 years ago by
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 6 years ago by
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 thanorthogonal_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 6 years ago by
- 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: ↓ 24 Changed 6 years ago by
- 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: ↓ 27 Changed 6 years ago by
- 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:
db9bd8d | trac #17034: assert has been declared illegal
|
comment:25 Changed 6 years ago by
- 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:
b046793 | trac #17034: assert has been declared illegal
|
comment:26 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:27 in reply to: ↑ 24 Changed 6 years ago by
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 6 years ago by
- Branch changed from public/17034 to b0467933bee693c0a71a83cba53af049abb0c23c
- Resolution set to fixed
- Status changed from positive_review to closed
comment:29 Changed 6 years ago by
- 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 6 years ago by
- 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:
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
|
comment:31 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:32 Changed 6 years ago by
- Branch changed from public/17034 to b9516b194fdbcd8ec5e17e6d251b63ef9ac3d7e8
- Resolution set to fixed
- Status changed from positive_review to closed
comment:33 Changed 4 years ago by
- 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.
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17034: New user interface for orthogonal arrays and a .explain_construction method