Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#19597 closed enhancement (fixed)

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

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.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 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.

Change History (18)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 5 years ago by jdemeyer

  • 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 jdemeyer

  • Branch set to u/jdemeyer/general_code_cleanup__avoid_x___eq___y_

comment:7 Changed 5 years ago by git

  • Commit set to b2094a8a1cedafab73db0ed7653db3112982cec4

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

b2094a8Avoid direct calls of special methods

comment:8 Changed 5 years ago by jdemeyer

  • Status changed from new to needs_review

comment:9 follow-ups: Changed 5 years ago by vdelecroix

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): 
    803803        self._hyp_refine_vals_scratch = <long *> sage_malloc(
    804804                            self._hyp_part.degree * sizeof(long))
    805805        if self._hyp_refine_vals_scratch is NULL:
    806             self.__dealloc__()
    807806            raise MemoryError('allocating PartitionRefinementLinearCode')
    808807
    809808        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.

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:10 follow-up: Changed 5 years ago by jdemeyer

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 jdemeyer

Replying to vdelecroix:

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

comment:12 in reply to: ↑ 10 Changed 5 years ago by jdemeyer

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.

comment:13 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

Replying to vdelecroix:

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.

comment:14 Changed 5 years ago by git

  • Commit changed from b2094a8a1cedafab73db0ed7653db3112982cec4 to 4a950ac85c2e083d93b2f5037b2c8ed329c25bea

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

4a950acMake __pow__ more efficient

comment:15 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:16 Changed 5 years ago by vbraun

  • 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 follow-up: Changed 5 years ago by dimpase

  • Commit 4a950ac85c2e083d93b2f5037b2c8ed329c25bea deleted

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

comment:18 in reply to: ↑ 17 Changed 5 years ago by jdemeyer

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.

Note: See TracTickets for help on using tickets.