Enhance the e_one_star.Patch class

[sagetrac-tjolivet]

A few enhancements for the Patch class:

• remove method;
• __hash__ method;
• deal more cleanly with construction of new patches (sometimes the same Face belonged to different patches, which caused problems with colors, for example).

For the patchbot (this info must be put here instead of below):

Apply:

• attachment: trac-11255-tj.patch
• attachment: trac-11255-tj-modifs.patch
• attachment: trac_11255_reviewer-sl.patch
• attachment: trac_11255_modifs2-tj.patch
• attachment: trac_11255_reviewer-2nd-sl.patch

CC: @seblabbe

Component: combinatorics

Author: Timo Jolivet

Reviewer: Sébastien Labbé

Merged: sage-4.7.1.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/11255

[sagetrac-tjolivet]

comment:1

Attachment: trac-11255-tj.patch.gz

For the patchbot :

Apply trac-11255-tj.patch

[seblabbe]

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 :

-       return (isinstance(other, Patch) and set(self) == set(other)) 
+ 	return (isinstance(other, Patch) and set(self._faces) == set(other._faces)) 

[sagetrac-tjolivet]

comment:5

Replying to @seblabbe:

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 :

-       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)

[seblabbe]

comment:6

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

[seblabbe]

comment:7

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...):

This problem would be solved by using a frozenset instead of a list for representing a Patch.

[sagetrac-tjolivet]

comment:8

Replying to @seblabbe:

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

[sagetrac-tjolivet]

applies over the first patch

[sagetrac-tjolivet]

comment:9

Attachment: trac-11255-tj-modifs.patch.gz

Dear Sébastien, I just added a Patch.__sub__ method, which acts as Patch.__difference__, I overrode trac-11255-tj-modifs.patch.

[seblabbe]

Attachment: trac_11255_reviewer-sl.patch.gz

Applies over the precedent 2 patches

[seblabbe]

comment:11

I just added a reviewer patch which applies over the precedent two. There was doctest errors with the hash method (I obtain different hash results on my machine).

I still have concerns with two things before giving a positive review : (1) I think the method Patch.__getitem__ should be removed as the result can be different on other machine and (2) the ordering of the iter method of a Patch is not guarenteed. So the result of the tikz method may change on other machine which may lead to doctest problem later. So I suggest not to change the tikz method, but rather change the way we test them.

[sagetrac-tjolivet]

comment:12

Hi, thanks for you corrections and your patch.

I still have concerns with two things before giving a positive review : (1) I think the method Patch.__getitem__ should be removed as the result can be different on other machine and (2) the ordering of the iter method of a Patch is not guarenteed. So the result of the tikz method may change on other machine which may lead to doctest problem later. So I suggest not to change the tikz method, but rather change the way we test them.

I would like to keep the __getitem__ method because we use it often when we need to pick an arbitrary face of a patch. But we should in this case write a WARNING in the doc, I agree that the result changing from machine to machine can be confusing.

I will soon make the changes to the doc to prevent the __iter__ ordering problems. (Should be easy by comparing strings or by using __sorted__. How do you think should the tikz output be tested to prevent these problems?

[sagetrac-tjolivet]

comment:13

Hi,

Four things:

I have solved all the doc problems related to the fact that Patch.__getitem__ doesn't necessarily return the same result from machine to machine. I would like to keep this convenient method (it is used in various other methods and I use it personally for some other tasks). I wrote a WARNING in the doc about this fact.

The plot_tikz doctesting problem related to the non-order of Patch.__iter__ has been solved.

I optimized a little the plot_tikz method, by printing a \definecolor only when a new color is needed.

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.

[seblabbe]

comment:14

Salut Timo,

Excuse-moi pour la dernière semaine, j'aurais dû te répondre avant.

I have solved all the doc problems related to the fact that Patch.__getitem__ doesn't necessarily return the same result from machine to machine. I would like to keep this convenient method (it is used in various other methods and I use it personally for some other tasks). I wrote a WARNING in the doc about this fact.

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...

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.

I optimized a little the plot_tikz method, by printing a \definecolor only when a new color is needed.

OK

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 :

- 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.

[seblabbe]

comment:15

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.

Again, add such an example in the doctests.

Sébastien

[sagetrac-tjolivet]

comment:16

Replying to @seblabbe:

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 :

- 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

[sagetrac-tjolivet]

Attachment: trac_11255_modifs2-tj.patch.gz

[seblabbe]

comment:17

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...).

To get the first element, you can do:

sage: S = set(range(10))             
sage: next(iter(S))     # or equivalently: iter(S).next()       
0  

To get the i-th element:

sage: from itertools import islice   
sage: next(islice(iter(S), 7, None)) 
7                                    

I look at your patch and review it soon. Je crois bien qu'on y est maintenant!

Sébastien

[seblabbe]

comment:18

I just upload a patch which applies over all the precedent ones. The positive
review is near. Again, I have one question about the methods
faces_of_vector, faces_of_type:

-    return [face for face in self if face.vector() == vector(v)] 
+    return [Face(f.vector(), f.type(), f.color()) for f in self if f.vector() == vector(v)] 

-    return [face for face in self if face.type() == t] 
+    return [Face(f.vector(), f.type(), f.color()) for f in self if f.type() == t] 

Why do we need copies in those cases?

Sébastien

[seblabbe]

Reviewer: Sébastien Labbé

[sagetrac-tjolivet]

comment:20

Replying to @seblabbe:

Why do we need copies in those cases?

Hi, if for some reason the user repaints the returned faces, we don't want it to change the color of the faces in the original Patch.

Otherwise, are you sure about the next(iter(P)) trick? The other seems faster:

sage: a = frozenset(range(10000))
sage: timeit('list(a)[0]')
625 loops, best of 3: 107 µs per loop
sage: timeit('next(iter(a))')
625 loops, best of 3: 607 ns per loop

[mwhansen]

comment:21

next(iter(a)) is faster. One of those timings is microseconds and the other is nanoseconds.

[seblabbe]

comment:22

I feel I should and could be cited as an author of the file e_one_star.py especially for the big modifications we made together on the file last year. I added my name in the last patch. Do you agree Timo?

Sébastien

[sagetrac-tjolivet]

comment:23

Replying to @seblabbe:

I feel I should and could be cited as an author of the file e_one_star.py especially for the big modifications we made together on the file last year. I added my name in the last patch. Do you agree Timo?

Of course! I thought so since the work we had done in Porquerolles but you wanted to be a reviewer instead. It's good that you added yourself.

Otherwise, I agree with the other changes of your last patch.

[seblabbe]

comment:24

Replying to @sagetrac-tjolivet:

Replying to @seblabbe:

Why do we need copies in those cases?

Hi, if for some reason the user repaints the returned faces, we don't want it to change the color of the faces in the original Patch.

What if the user wants to get the faces of a certain type and does not want to repaint them? We do useless copies in that case. I feel like those methods should return the faces as they are. I think the user may make the copies himself.

It is like if the patch was a Python set. Iterating through it iterates through its elements (not copies of them).

sage: f = Face((0,0,0), 1, color='red')      
sage: g = copy(f)                            
sage: g.color()                              
RGB color (1.0, 0.0, 0.0)                    
sage: f.color('black')                       
sage: g.color()                              
RGB color (1.0, 0.0, 0.0)                    
sage: f.color()                              
RGB color (0.0, 0.0, 0.0)                    

Sébastien

[sagetrac-tjolivet]

comment:25

Replying to @seblabbe:

What if the user wants to get the faces of a certain type and does not want to repaint them? We do useless copies in that case. I feel like those methods should return the faces as they are. I think the user may make the copies himself.

It is like if the patch was a Python set. Iterating through it iterates through its elements (not copies of them).

OK, I agree. If Patch.__iter__ doesn't return new copies then Patch.faces_of_truc should behave the same. (And some user might also want to change the colors of the faces of given vector or type.)

Oh, I think we should also add a method Patch.faces_of_color. We just have to be careful about the input: we must convert it in the same way as we convert face colors, and we must take into account that color comparison is python is a bit strange:

sage: a = Color('red')
sage: b = Color('red')
sage: a == b
False
sage : tuple(a) == tuple(b)
True

[seblabbe]

Attachment: trac_11255_reviewer-2nd-sl.patch.gz

Applies over the precedent patches

[seblabbe]

comment:26

I am ready to give a positive review to this ticket. But before, Timo, you must check my last patch.

Sébastien

[sagetrac-tjolivet]

comment:27

Replying to @seblabbe:

I am ready to give a positive review to this ticket. But before, Timo, you must check my last patch.

Very good, I agree! Thanks a lot.

[jdemeyer]

Merged: sage-4.7.1.alpha3