#11255: Enhance the e_one_star.Patch class
-----------------------------+----------------------------------------------
Reporter: tjolivet | Owner: sage-combinat
Type: enhancement | Status: new
Priority: major | Milestone: sage-4.7.1
Component: combinatorics | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author: Timo Jolivet
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Comment(by slabbe):
Salut Timo,
Below are my comments.
> * `remove` method;
Instead of writing a sentence about the input, I prefer if we follow the
Sage Developper's Guide and write an INPUT section.
No need to import E1Star in the doctest of the remove method.
> * `__hash__` method;
I have some concerns about adding a hash method to the Patch class. Since
a patch can be modified (by using the remove method for instance), strange
behavior can occur like the following :
{{{
sage: P = Patch(...)
sage: Q = copy(P)
sage: S = set()
sage: S.add(P)
sage: P.remove(some face A)
sage: Q.remove(some face A)
sage: Q in S
False
sage: Q == S.pop()
True
}}}
Any serious Python or Sage object is either mutable and unhashable or
immutable and hashable. In Sage, matrices and vector are allowed to pass
from one state to the other using methods like {{{set_immutable}}} and
{{{set_mutable}}}. This issue must be fixed.
> * deal more cleanly with construction of new patches (sometimes the
same Face belonged to different patches, which caused problems with
colors, for example).
Can you provide an example that justifies the modification? Without an
example, I can not say that I agree with the solution. I feel like the
solution is doing too much copies. But, I can't suggest an alternative,
since I do not know what the problem really is.
Once the problem is solved, such an example illustrating the problem you
mention must be added as a doctest.
And finally, I do not understand this modification :
{{{
#!diff
- return (isinstance(other, Patch) and set(self) == set(other))
+ return (isinstance(other, Patch) and set(self._faces) ==
set(other._faces))
}}}
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:2>
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.
