#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 slabbe):
> 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.
If we want Patches to be keys of dictionnary, vertices of graphs or
elements of
sets, it must be hashable. The easiest solution I see is that an object
Patch
be always hashable and never mutable. That means we made a mistake earlier
by
allowing to modify a Patch. I am responsible for this as I remember I
disliked
methods like {{{apply_substitution}}} but I were not able to verbalize it
and
relate this with the ability of hashing the object.
There are three methods that change self : {{{add}}},
{{{apply_substitution}}}
and {{{repaint}}}. I think we can keep {{{repaint}}} as it does not change
the
mathematical object and hence won't affect the result of the hash method
(a
doctest making sure of that the hash method is independant of color
changes
should be added). Surprisingly, we did not made the mistake with the
method
{{{translate}}} which does not change self and return a new patch. I think
it
is easy to remove the method {{{apply_substitution}}} since (1) one can
apply a
substitution by just applying a substiution directly on it : ``E(P)`` and
(2)
the only reason I can understand the existence of the method
{{{apply_substitution}}} is that it is faster than doing ``E(P)`` which is
not
the case anyway since it calls ``E(P)`` anyway. I also think the method
{{{add}}} can be easily removed. We should replace it by a method
{{{__add__}}} or {{{union}}} which adds two patches and return a new
patch.
Now, the method {{{adds}}} and {{{apply_substitution}}} should not be
removed
right now. Deprecation warnings should be added. Well usually, a
deprecation
warning should be raised for one year before removing the method. But,
since I
believe fixing the immutable/hashable issue is very important. I think
their
behavior could be changed now. A deprecation warning saying something like
:
"Object sage.combinat.e_one_star.Patch are immutable since Sage-4.7.1. Use
the
usual addition instead which returns a new object: P + Q." A similar
warning
should be added to the method {{{apply_substitution}}}.
> 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.
One can create a Patch from a (1) iterable of faces or (2) from a Patch.
The
problem comes from the case (2) were a copy should be done. Here is my
first
suggestion:
{{{
if isinstance(faces, Patch):
self._faces = [Face(f.vector(), f.type(), f.color()) for f in faces]
else:
self._faces = list(faces)
}}}
> 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)
> }}}
First, there is a problem with the method {{{remove}}} because it changes
the
Patch. Compare the methods and their name of the mutable unhashable Python
set
and the immutable and hashable Python frozenset :
{{{
sage: python_set = set([])
sage: [method for method in dir(python_set) if not method.startswith('_')]
['add', 'clear', 'copy', 'difference', 'difference_update', 'discard',
'intersection', 'intersection_update', 'isdisjoint', 'issubset',
'issuperset',
'pop', 'remove', 'symmetric_difference', 'symmetric_difference_update',
'union', 'update']
sage: frozen_set = frozenset()
sage: [method for method in dir(frozen_set) if not method.startswith('_')]
['copy', 'difference', 'intersection', 'isdisjoint', 'issubset',
'issuperset',
'symmetric_difference', 'union']
}}}
Hence, I suggest to implement a method called {{{difference}}} which
returns a
new Patch instead of the method remove.
My last question concerns the representation of the Patch. Now, we
represent a
patch as a list of faces. I think this choice was made because we wanted
an
object Patch to be mutable without loosing time on unicity of faces
concerns
which did not bothered us, since it is guarenteed mathematically (well at
least
for the application of E1Stars). But, if a Patch is now immutable, maybe a
Python frozenset or maybe a Sage Set (also immutable) would be better.
{{{
sage: sage_set = Set([])
sage: [method for method in dir(sage_set) if not method.startswith('_')]
['CartesianProduct', 'Hom', 'algebra', 'an_element', 'base', 'base_ring',
'cardinality', 'cartesian_product', 'categories', 'category', 'coerce',
'coerce_embedding', 'coerce_map_from', 'construction', 'convert_map_from',
'db', 'difference', 'dump', 'dumps', 'element_class', 'frozenset',
'gens_dict',
'get_action', 'has_base', 'has_coerce_map_from', 'hom',
'inject_variables',
'injvar', 'intersection', 'is_exact', 'is_finite', 'latex_name',
'latex_variable_names', 'list', 'normalize_names', 'object', 'objgen',
'objgens', 'register_action', 'register_coercion', 'register_conversion',
'register_embedding', 'rename', 'reset_name', 'save', 'set',
'some_elements',
'subsets', 'symmetric_difference', 'union', 'variable_name',
'variable_names',
'version']
}}}
What do you think?
Sébastien
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:6>
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.
