#11255: Enhance the e_one_star.Patch class
-----------------------------+----------------------------------------------
Reporter: tjolivet | Owner: sage-combinat
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.7.1
Component: combinatorics | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author: Timo Jolivet
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Changes (by slabbe):
* status: needs_review => needs_work
Comment:
Salut Timo,
Excuse-moi pour la dernière semaine, j'aurais dû te répondre avant.
> I have solved all the doc problems related to the fact that
{{{Patch.__getitem__}}} doesn't necessarily return the same result from
machine to machine. I would like to keep this convenient method (it is
used in various other methods and I use it personally for some other
tasks). I wrote a WARNING in the doc about this fact.
I still have problem with that. By implementing {{{__getitem__}}} you kind
of tell the user that this is a good method to use. Indeed it is practical
to use the square bracquets. The problem is that the method is very slow.
It makes a list everytime it is called. Suppose for instance the user use
getitem 5 times in a row (which is natural). This will create 5 times the
same list. I still believe the method {{{__getitem__}}} should be removed.
If the user wants to get the 5th element, he will create the list himself
and that's it. This is better and avoids misunderstanding of the
representation. It is a set. It behaves as a set. And if you want the i-th
element of a python set, what do you do? The same thing.
I have another argument. Not only it may affect the doctests depending on
the machine as I said earlier. But also, and more importantly, it affects
the fiability of the code someone write. I know you know the warning. But
such a warning is not natural for the method getitem. Hence, some user
will write code, send it to a friend, and it might be broken.
In other words, implement {{{__getitem__}}} only if it constant time and
of course if its result is guarenteed...
> The {{{plot_tikz}}} doctesting problem related to the non-order of
{{{Patch.__iter__}}} has been solved.
You solved it by just adding {{{ # not tested }}} right? You should add
{{{ # random }}} instead so the code will be tested (makes sure no error)
but not the result.
> I optimized a little the {{{plot_tikz}}} method, by printing a
{{{\definecolor}}} only when a new color is needed.
OK
> Also, I have been more careful about making new Faces when creating new
Patches. It is not enough to create new Faces only when
{{{isinstance(other, Patch)}}} in {{{Patch.__add__}}}: for example, if we
get a list L of faces that belong to a patch P, and then create the patch
Q + L, the faces of L have to be copied, otherwise changing the color of Q
will also change some colors of P. I have solved all instances of this
potential problem.
OK. Fine. But now, I think we are doing too much. How many copies of
copies do we need to be safe? I think one is enough. For instance, here :
{{{
#!diff
- return Patch(self._faces.difference(other))
+ P = Patch(self)
+ Q = Patch(other)
+ return Patch(P._faces.difference(Q))
}}}
The line {{{return Patch(self._faces.difference(other))}}} is OK if the
copy is always done by the {{{__init__}}} method. Note that this is done
at least 3 times in the code.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:14>
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 post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
