#11255: Enhance the e_one_star.Patch class
-----------------------------+----------------------------------------------
Reporter: tjolivet | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.7.1
Component: combinatorics | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author: Timo Jolivet
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Changes (by tjolivet):
* status: needs_work => needs_review
Comment:
Replying to [comment:14 slabbe]:
> 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...
>
OK, there is no more {{{Patch.__getitem__}}} method. The only thing that
bugs me is that we have to create a whole iterable to pick a single
element of a Patch: the best way I can think of is {{{list(P)[0]}}}. Using
{{{random.choice}}} is of course overkill, and there is no
{{{frozenst.pick_arbitrary}}} method (which would be nice...). I have
added a {{{Patch.dimension()}}} method to avoid picking a face everytime
we want to check the dimension of Patch. Hence, faces are "picked" two
times in the code: in {{{Patch.__init__}}}, and in
{{{Patch.occurences_of}}}.
> > 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.
>
No, the code is tested as it should (even the length of the returned
string is tested). Only {{{print s}}} is not tested.
> > 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.
There were indeed too many copies, I changed that.
I uploaded a new version of the last patch.
Cheers,
Timo
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:16>
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.
