#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:
-----------------------------+----------------------------------------------
Comment(by tjolivet):
Replying to [comment:2 slabbe]:
> Instead of writing a sentence about the input, I prefer if we follow the
Sage Developper's Guide and write an INPUT section.
I agree, I corrected this.
> No need to import E1Star in the doctest of the remove method.
Thanks for mentionning this. I corrected many other instances of the same
issue.
> 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.
Here is why I added a `__hash__` method to Patch:
{{{
sage: P = Patch([Face([0,0,0],1), Face([0,0,0],2)])
sage: Q = Patch([Face([0,0,0],2), Face([0,0,0],1)])
sage: P == Q
True
sage: hash(P)
1297676529065262660
sage: hash(Q)
-8173426908364432914
}}}
I had a lot of problems because of this when I used some `DiGraphs` whose
vertices were `Patches`. If you have a better solution, let me know. I
added it in the doctest under a TEST section.
> 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.
Here is the problem with colors if we don't create new faces in
Patch.__init__:
{{{
sage: P = Patch([Face([0,0,0],1), Face([0,0,0],2)])
sage: Q = Patch(P)
sage: P[0].color()
RGB color (1.0, 0.0, 0.0)
sage: Q[0].color('yellow')
sage: P[0].color()
RGB color (1.0, 1.0, 0.0)
}}}
Let me know if you have a better solution. I added it in the doctest under
a TEST section.
> 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))
> }}}
OK, I reverted this useless modification (something I had tested but
forgot to remove).
Please tell me if you agree with what I said concerning the main two
issues you raised (`__hash__` and creating new faces in `Patch.__init__`).
I will upload a new patch if you confirm.
Also, do you think the following code of `Patch.remove` can be made
better? (It looks pretty naive but I'm not sure if using `set` would be
more efficient...):
{{{
if isinstance(faces, Face):
while faces in self._faces:
self._faces.remove(faces)
else:
for f in faces:
while f in self._faces:
self._faces.remove(f)
}}}
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:5>
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.
