#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:6 slabbe]:
> 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}}}.
OK, these methods now return a new patch, along with a deprecation
warning.
I changed the doc in favor of using {{{E1Star.__call__}}} insead of
{{{apply_substitution}}}.
> 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)
> }}}
This is a good solution, I use it.
> 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.
OK, I this is also a better solution. I also added a {{{union}}} method,
that does the job that {{{Patch.add}}} was supposed to do (so that the
deprecation warning of {{{add}}} is not annoying when {{{__add__}}} is
used). The method {{{remove}}} no longer exists.
> 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?
Everything is cleaner if we use sets instead of lists, you're right. I
submitted a new patch that uses frozenset and made the according changes
in the doc and everywhere, which means quite a lot of stuff as you will
see in my new patch, which applies over the previous one.
Also, I added an option in {{{E1Star.__init__}}}: we can now choose
between the 'suffix' or 'prefix' version of the dual substitution. The
default one ('suffix') is the one that was used before. Adding this is
important because both versions are widely used in the literature. (I also
moved {{{_base_iter}}} from a method to a direct creation in
{{{__init__}}}, the gain of time given by {{{@lazy_attribute}}} was really
negligible.)
Thank you for your very useful corrections.
For the patchbot :
Apply trac-11255-tj.patch
Apply trac-11255-tj-modifs.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11255#comment:8>
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.
