#19597 closed enhancement (fixed)
General code cleanup: avoid code like x.__eq__(y)
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  4a950ac (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Replace code of the form
x.__eq__(y) x.__len__() x.__getitem__(y) x.__contains__(y) ...
by
x == y len(x) x[y] y in x ...
because the latter are more efficient.
The obvious exceptions are super()
calls and Class.__method__(...)
calls.
In a few cases, we reorganize the code to avoid special functions/methods altogether:
 for
__repr__()
, we instead change%s
to%r
for%
formatting and{}
to{!r}
for.format()
.  certain calls to
__iter__()
can be changed to a simple list comprehension.  there are subtle differences between
x.__eq__(y)
andx == y
and in certain technical cases they do matter.
Change History (18)
comment:1 Changed 5 years ago by
 Description modified (diff)
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Description modified (diff)
comment:4 Changed 5 years ago by
 Description modified (diff)
comment:5 Changed 5 years ago by
 Description modified (diff)
 Summary changed from General code cleanup: avoid x.__eq__(y) to General code cleanup: avoid code like x.__eq__(y)
comment:6 Changed 5 years ago by
 Branch set to u/jdemeyer/general_code_cleanup__avoid_x___eq___y_
comment:7 Changed 5 years ago by
 Commit set to b2094a8a1cedafab73db0ed7653db3112982cec4
comment:8 Changed 5 years ago by
 Status changed from new to needs_review
comment:9 followups: ↓ 11 ↓ 13 Changed 5 years ago by
Could you comment more on this two changes

src/sage/coding/codecan/codecan.pyx
diff git a/src/sage/coding/codecan/codecan.pyx b/src/sage/coding/codecan/codecan.pyx index 4d092d1..7b0bec3 100644
a b cdef class PartitionRefinementLinearCode(PartitionRefinement_generic): 803 803 self._hyp_refine_vals_scratch = <long *> sage_malloc( 804 804 self._hyp_part.degree * sizeof(long)) 805 805 if self._hyp_refine_vals_scratch is NULL: 806 self.__dealloc__()807 806 raise MemoryError('allocating PartitionRefinementLinearCode') 808 807 809 808 self._hyp_refine_vals = _BestValStore(self._hyp_part.degree)
diff git a/src/sage/combinat/crystals/tensor_product.py b/src/sage/combinat/crystals/tensor_product.py index 9596832..f3d277b 100644  a/src/sage/combinat/crystals/tensor_product.py +++ b/src/sage/combinat/crystals/tensor_product.py @@ 199,9 +199,7 @@ class ImmutableListWithParent(CombinatorialElement): sage: m <= n True """  if self == other:  return True  return self.__lt__(other) + return self == other or self.__lt__(other) def __gt__(self, other): @@ 237,9 +235,7 @@ class ImmutableListWithParent(CombinatorialElement): sage: m >= n False """  if self == other:  return True  return self.__gt__(other) + return self == other or self.__gt__(other) def sibling(self, l):
More comments:
combinat/root_system/weyl_characters.py
: a very strange way to compute powers self * self ** (n1)
. I guess that generic power would be better here!
groups/perm_gps/permgroup.py
: Is there any difference between list(L)
and [x for x in L]
? As far as I understand list
is smarter and call for __len__
to allocate directly the right amount of memory. Whereas the second uses .append
.
comment:10 followup: ↓ 12 Changed 5 years ago by
About __dealloc__
: there is no __dealloc__
method, it's a "fake" Cython method just like __cinit__
. It's just not possible to call it.
In src/sage/combinat/crystals/tensor_product.py
, you are probably wondering why I still use __lt__
instead of <
for example. That's because the __lt__
method can return NotImplemented
. In this case, the output of x < y
is not the same as x.__lt__(y)
.
comment:11 in reply to: ↑ 9 Changed 5 years ago by
Replying to vdelecroix:
groups/perm_gps/permgroup.py
: Is there any difference betweenlist(L)
and[x for x in L]
? As far as I understandlist
is smarter and call for__len__
to allocate directly the right amount of memory. Whereas the second uses.append
.
That's true in general, however: there is no __len__
implemented in this case. There is a generic __len__
coming from Parent
but that calls len(self.list())
which would lead to an infinite loop. So I think that [x for x in L]
is the most efficient way to create the list here.
That's probably also the reason the old code was written like list(self.__iter__())
and not list(self)
.
comment:12 in reply to: ↑ 10 Changed 5 years ago by
Replying to jdemeyer:
In
src/sage/combinat/crystals/tensor_product.py
, you are probably wondering why I still use__lt__
instead of<
for example. That's because the__lt__
method can returnNotImplemented
. In this case, the output ofx < y
is not the same asx.__lt__(y)
.
Let me also mention that this is the only place in Sage where a comparison method __lt__
, __gt__
, __le__
or __ge__
returns NotImplemented
.
In src/sage/combinat/words/abstract_word.py
, there is an __eq__
which can return NotImplemented
, but I don't think there is a problem there: I believe calling self == other
in __ne__
is still the right thing to do.
And there are several other places where a __richcmp__
can return NotImplemented
but those are not affected by this ticket.
comment:13 in reply to: ↑ 9 Changed 5 years ago by
Replying to vdelecroix:
combinat/root_system/weyl_characters.py
: a very strange way to compute powersself * self ** (n1)
. I guess that generic power would be better here!
It's apparently intentional. The doc says that it's more efficient to compute a*(a*(a*a))
than (a*a)*(a*a)
. Still, I can change the code to use a loop instead of recursion.
comment:14 Changed 5 years ago by
 Commit changed from b2094a8a1cedafab73db0ed7653db3112982cec4 to 4a950ac85c2e083d93b2f5037b2c8ed329c25bea
Branch pushed to git repo; I updated commit sha1. New commits:
4a950ac  Make __pow__ more efficient

comment:15 Changed 5 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
comment:16 Changed 5 years ago by
 Branch changed from u/jdemeyer/general_code_cleanup__avoid_x___eq___y_ to 4a950ac85c2e083d93b2f5037b2c8ed329c25bea
 Resolution set to fixed
 Status changed from positive_review to closed
comment:17 followup: ↓ 18 Changed 5 years ago by
 Commit 4a950ac85c2e083d93b2f5037b2c8ed329c25bea deleted
that was a bit too quick, and broke some optional doctests, see https://groups.google.com/d/msg/sagerelease/rLaGmnQFgY/mR91V4K0BAAJ
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Avoid direct calls of special methods