#16286: Allow k=None in MOLS/TD/OA
-------------------------------------+-------------------------------------
Reporter: ncohen | Owner:
Type: enhancement | Status: positive_review
Priority: major | Milestone: sage-6.2
Component: combinatorics | Resolution:
Keywords: design, mols, | Merged in:
orthogonal array | Reviewers: Vincent Delecroix
Authors: Nathann Cohen | Work issues:
Report Upstream: N/A | Commit:
Branch: | a97434f3264d183056a50ac54fcb7a9912d31efc
u/vdelecroix/16286 | Stopgaps:
Dependencies: #16272 |
-------------------------------------+-------------------------------------
Changes (by ncohen):
* status: needs_work => positive_review
* reviewer: => Vincent Delecroix
* branch: u/ncohen/16286 => u/vdelecroix/16286
* commit: 4970da45741acac330b2602c967e5fed681fdfa3 =>
a97434f3264d183056a50ac54fcb7a9912d31efc
Comment:
Yo !
> I run through the code and mainly changed some documentation. It is
available at u/vdelecroix/16286 if you want to have a look it and possibly
adopt it.
Yep, no problem.
> It becomes crazy: the functions return either a design , a troolean or
an integer depending on the input...
True, but I think that without reading the doc you can guess what you will
find, or at least correctly interpret the output. So well, doesn't matter
;-)
Besides, with the doctest you quickly see what it means.
> I added an OUTPUT section to make it clearer (only in OA/TD). Moreover I
am not sure I like the NOTE inside the INPUT. On the one hand, it is
unreadable in the console. On the other hand, in the html output the NOTE
has a gray background and makes your eyes focus on it... but it is by far
not the most important thing.
Ahahaahah. Well, to be honest I just want all these patches to be merged,
then we will clean stuff. Each time you add a doctest I have to rewrite it
one thousand times because the new constructions break it. It's over-
documented and over-tested at the moment I believe, but it is not a
problem. We have time.
> We forgot two `ValueError` that should be `EmptySetError` inside the
MOLS constructor. I changed it.
Thanks ! I am still not used to those `EmptySetError`
> Inside the TESTS of transversal design, you modified the loop to not
test the case where the function should return `True`. As the loop is
designed to '''test''' if it the case I did the change backward.
Nevertheless, I added a line that checks that we got the correct value
when `k=None`.
You are testing a loop ? Yes of course, everything can fail.
> I added more examples and hope that what does the function looks clearer
when somebody reads the documentation. In particular, most of the TESTS
section are now useless and I just removed them.
OKayyyyyyyy. Well for me it is good to go, too. I am eager to see it
stabilize, right now I have so many versions of this code in my head that
I barely know what is already there and what is left to implement.
Thanks !
Nathann
----
New commits:
||[http://git.sagemath.org/sage.git/commit/?id=a97434f3264d183056a50ac54fcb7a9912d31efc
a97434f]||{{{trac #16286: more doc, more tests}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/16286#comment:7>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
