Opened 10 years ago
Closed 10 years ago
#11255 closed enhancement (fixed)
Enhance the e_one_star.Patch class
Reported by: | tjolivet | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | combinatorics | Keywords: | |
Cc: | slabbe | Merged in: | sage-4.7.1.alpha3 |
Authors: | Timo Jolivet | Reviewers: | Sébastien Labbé |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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:
Attachments (5)
Change History (35)
Changed 10 years ago by
comment:1 Changed 10 years ago by
comment:2 in reply to: ↑ description ; follow-up: ↓ 5 Changed 10 years ago by
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))
comment:3 Changed 10 years ago by
- Status changed from new to needs_review
comment:4 Changed 10 years ago by
- Status changed from needs_review to needs_work
comment:5 in reply to: ↑ 2 ; follow-ups: ↓ 6 ↓ 7 Changed 10 years ago by
Replying to 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() TrueAny 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
andset_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)
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 10 years ago by
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) -8173426908364432914I had a lot of problems because of this when I used some
DiGraphs
whose vertices werePatches
. 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 inPatch.__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 usingset
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
comment:7 in reply to: ↑ 5 Changed 10 years ago by
Also, do you think the following code of
Patch.remove
can be made better? (It looks pretty naive but I'm not sure if usingset
would be more efficient...):
This problem would be solved by using a frozenset instead of a list for representing a Patch.
comment:8 in reply to: ↑ 6 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to 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
andrepaint
. I think we can keeprepaint
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 methodtranslate
which does not change self and return a new patch. I think it is easy to remove the methodapply_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 doingE(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__
orunion
which adds two patches and return a new patch.Now, the method
adds
andapply_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 methodapply_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
comment:9 Changed 10 years ago by
Dear Sébastien, I just added a Patch.__sub__
method, which acts as Patch.__difference__
, I overrode trac-11255-tj-modifs.patch.
comment:10 Changed 10 years ago by
- Description modified (diff)
comment:11 follow-up: ↓ 12 Changed 10 years ago by
- Status changed from needs_review to needs_work
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.
comment:12 in reply to: ↑ 11 Changed 10 years ago by
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?
comment:13 follow-ups: ↓ 14 ↓ 15 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
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.
comment:14 in reply to: ↑ 13 Changed 10 years ago by
- Status changed from needs_review to needs_work
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 ofPatch.__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)
inPatch.__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.
comment:15 in reply to: ↑ 13 Changed 10 years ago by
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
comment:16 follow-up: ↓ 17 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to slabbe:
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 ofPatch.__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)
inPatch.__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
Changed 10 years ago by
comment:17 in reply to: ↑ 16 Changed 10 years ago by
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]
. Usingrandom.choice
is of course overkill, and there is nofrozenst.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
comment:18 Changed 10 years ago by
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
comment:19 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Sébastien Labbé
comment:20 follow-up: ↓ 24 Changed 10 years ago by
Replying to slabbe:
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
comment:21 Changed 10 years ago by
next(iter(a))
is faster. One of those timings is microseconds and the other is nanoseconds.
comment:22 follow-up: ↓ 23 Changed 10 years ago by
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
comment:23 in reply to: ↑ 22 Changed 10 years ago by
Replying to slabbe:
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.
comment:24 in reply to: ↑ 20 ; follow-up: ↓ 25 Changed 10 years ago by
Replying to tjolivet:
Replying to slabbe:
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
comment:25 in reply to: ↑ 24 Changed 10 years ago by
- Owner changed from sage-combinat to (none)
Replying to slabbe:
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
comment:26 follow-up: ↓ 27 Changed 10 years ago by
I am ready to give a positive review to this ticket. But before, Timo, you must check my last patch.
Sébastien
comment:27 in reply to: ↑ 26 Changed 10 years ago by
Replying to slabbe:
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.
comment:28 Changed 10 years ago by
- Status changed from needs_review to positive_review
comment:29 Changed 10 years ago by
- Description modified (diff)
comment:30 Changed 10 years ago by
- Merged in set to sage-4.7.1.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
For the patchbot :
Apply trac-11255-tj.patch