General code cleanup: avoid code like x.__eq__(y)

[jdemeyer]

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:

1. for __repr__(), we instead change %s to %r for %-formatting and {} to {!r} for .format().
2. certain calls to __iter__() can be changed to a simple list comprehension.
3. there are subtle differences between x.__eq__(y) and x == y and in certain technical cases they do matter.

Component: misc

Author: Jeroen Demeyer

Branch: 4a950ac

Reviewer: Vincent Delecroix

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

[jdemeyer]

Branch: u/jdemeyer/general_code_cleanup__avoid_x___eq___y_

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b2094a8

Avoid direct calls of special methods

[sagetrac-git]

Commit: b2094a8

[videlec]

comment:9

Could you comment more on this two changes

diff --git a/src/sage/coding/codecan/codecan.pyx b/src/sage/coding/codecan/codecan.pyx
index 4d092d1..7b0bec3 100644
--- a/src/sage/coding/codecan/codecan.pyx
+++ b/src/sage/coding/codecan/codecan.pyx
@@ -803,7 +803,6 @@ cdef class PartitionRefinementLinearCode(PartitionRefinement_generic):
         self._hyp_refine_vals_scratch = <long *> sage_malloc(
                             self._hyp_part.degree * sizeof(long))
         if self._hyp_refine_vals_scratch is NULL:
-            self.__dealloc__()
             raise MemoryError('allocating PartitionRefinementLinearCode')
 
         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 ** (n-1). 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.

[jdemeyer]

comment:10

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

[jdemeyer]

comment:11

Replying to @videlec:

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.

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

[jdemeyer]

comment:12

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 return NotImplemented. In this case, the output of x < y is not the same as x.__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.

[jdemeyer]

comment:13

Replying to @videlec:

combinat/root_system/weyl_characters.py: a very strange way to compute powers self * self ** (n-1). 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.

[sagetrac-git]

Changed commit from b2094a8 to 4a950ac

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

4a950ac

Make `__pow__` more efficient

[videlec]

Reviewer: Vincent Delecroix

[vbraun]

Changed branch from u/jdemeyer/general_code_cleanup__avoid_x___eq___y_ to 4a950ac

[dimpase]

Changed commit from 4a950ac to none

[dimpase]

comment:17

that was a bit too quick, and broke some optional doctests, see
https://groups.google.com/d/msg/sage-release/rLaGmnQ-FgY/mR91V4K0BAAJ

[jdemeyer]

comment:18

Replying to @dimpase:

that was a bit too quick, and broke some optional doctests

Well, there wasn't a single non-optional doctest for str(<libgap object>).

The issue is fixed at #19733 and needs review.