Skip to content

Conversation

@mantepse
Copy link
Contributor

@mantepse mantepse commented Mar 5, 2023

📚 Description

Instead of assigning to __iter__, we create a local class overriding __iter__. Thus,

sage: _ = Permutations(9).tuple()

will now create a cache containing all permutations of 9, which is actually used when iterating over Permutations(9).

Closes #35212

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

None

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

@tmller, that's for you! (more coming in another pull request)

@mantepse mantepse requested a review from tscrim March 5, 2023 11:39
@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2023

We probably should also test this for extension classes (e.g., cdef class in Cython), which do not get dynamically created classes. It might not work for them, which is okay, as long as it doesn't break them. However, nearly all of our subclasses of Parent are Python classes because we use multiple inheritance with UniqueRepresentation. I might just need to run a test locally to verify; I am pretty sure it's fine...

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

There are failures which I have to investigate. The one in finite_enumerated_sets.py was sort of expected, I made the test stricter than it was originally. I'll check the other ones now.

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

The failure in sage.combinat.integer_lists.lists.IntegerLists.__eq__ is serious, we are testing for equality of __class__ there:

    def __eq__(self, other):
...
        if self.__class__ != other.__class__:
            return False
        if self.backend != other.backend:
            return False
        a = self._element_constructor_
        b = other._element_constructor_
        if ismethod(a):
            a = a.__func__
        if ismethod(b):
            b = b.__func__
        return a == b

We do this in a few places:

grep -r --include=*.{py,pyx} --color=auto -nH --null -e "__class__ [=!]=" *
categories/pushout.py:3695:        if self.__class__ != other.__class__:
combinat/ordered_tree.py:1156:            sage: S.first().__class__ == OrderedTrees_all().first().__class__
combinat/subset.py:269:        if self.__class__ != other.__class__:
combinat/subset.py:645:        if self.__class__ != other.__class__:
combinat/subset.py:996:        if self.__class__ != other.__class__:
combinat/subset.py:1212:        if self.__class__ != other.__class__:
combinat/root_system/weyl_group.py:796:        return (self.__class__ == other.__class__ and
combinat/integer_lists/lists.py:169:        if self.__class__ != other.__class__:
combinat/rooted_tree.py:760:            sage: S.first().__class__ == RootedTrees().first().__class__
combinat/crystals/letters.pyx:213:        if value.__class__ == self.element_class and value.parent() is self:
combinat/species/structure.py:195:        if self.__class__ != x.__class__:
combinat/composition.py:879:            sage: c.fatten(Composition([3,1,1])).__class__ == c.__class__
combinat/subword.py:188:        return self.__class__ == other.__class__ and self._w == other._w and self._build == other._build
combinat/interval_posets.py:3841:            sage: S.first().__class__ == TamariIntervalPosets().first().__class__
combinat/set_partition_ordered.py:545:            sage: c.fatten(Composition([2,1])).__class__ == c.__class__
matrix/matrix_generic_sparse.pyx:283:            sage: (A+B).__class__ == A.__class__
matrix/matrix_generic_sparse.pyx:285:            sage: (B+A).__class__ == A.__class__
matrix/matrix_generic_sparse.pyx:287:            sage: (A+C).__class__ == A.__class__
matrix/matrix_generic_sparse.pyx:289:            sage: (C+A).__class__ == A.__class__
matrix/matrix_generic_sparse.pyx:291:            sage: (A+D).__class__ == D.__class__
misc/call.py:88:        return self.__class__ == other.__class__ and self.__dict__ == other.__dict__
misc/test_nested_class.py:124:        return self.__class__ == other.__class__
misc/test_nested_class.py:134:        return self.__class__ != other.__class__
misc/sageinspect.py:2476:                    if obj.__class__ != type:
symbolic/constants.py:315:        if self.__class__ == other.__class__ and self._name == other._name:
symbolic/function.pyx:1108:        if sfunc.__class__ == self.__class__:

Grep finished with 26 matches found at Sun Mar  5 16:54:46

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

The failures in sage.combinat.posets.posets.FinitePoset.linear_extensions and sage.combinat.parking_functions.ParkingFunctions are superficial, they only concern the name of the new class, I am not sure what it should be.

The failures in sage.combinat.shuffle are about pickling. I know nothing about this stuff.

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

Does the following look correct?

diff --git a/src/sage/combinat/integer_lists/lists.py b/src/sage/combinat/integer_lists/lists.py
index 5143e5cb5e8..0db3ccd27c6 100644
--- a/src/sage/combinat/integer_lists/lists.py
+++ b/src/sage/combinat/integer_lists/lists.py
@@ -166,7 +166,7 @@ class IntegerLists(Parent):
             ....:     lambda n: IntegerListsLex(n, length=1))).list()
             [[2], [2]]
         """
-        if self.__class__ != other.__class__:
+        if not isinstance(other, IntegerLists):
             return False
         if self.backend != other.backend:
             return False

@mantepse
Copy link
Contributor Author

mantepse commented Mar 5, 2023

The problem in sage.combinat.shuffle boils down to a problem in equality. However, here I am even less sure than above:

diff --git a/src/sage/combinat/shuffle.py b/src/sage/combinat/shuffle.py
index 4c8a9cdd217..a5727312d37 100644
--- a/src/sage/combinat/shuffle.py
+++ b/src/sage/combinat/shuffle.py
@@ -135,7 +135,7 @@ class ShuffleProduct_abstract(Parent):
             sage: SP == ShuffleProduct([1,2],[4,5,7])
             False
         """
-        if not isinstance(other, type(self)):
+        if not isinstance(other, ShuffleProduct_abstract):
             return False
         return (self._l1 == other._l1 and self._l2 == other._l2)

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 97.99% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (357edbc) 88.59%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35232      +/-   ##
===========================================
+ Coverage    88.57%   88.59%   +0.01%     
===========================================
  Files         2140     2140              
  Lines       397273   397420     +147     
===========================================
+ Hits        351891   352076     +185     
+ Misses       45382    45344      -38     
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py 93.11% <66.66%> (-0.12%) ⬇️
src/sage/interfaces/tachyon.py 87.93% <90.00%> (+0.43%) ⬆️
src/sage/schemes/elliptic_curves/gal_reps.py 82.23% <90.00%> (+0.04%) ⬆️
src/sage/quadratic_forms/quadratic_form.py 90.26% <95.65%> (+0.16%) ⬆️
src/sage/categories/finite_enumerated_sets.py 96.89% <100.00%> (+0.09%) ⬆️
src/sage/modular/quasimodform/element.py 99.20% <100.00%> (+0.06%) ⬆️
src/sage/rings/qqbar.py 95.30% <100.00%> (+<0.01%) ⬆️
src/sage/schemes/affine/affine_morphism.py 90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py 43.75% <100.00%> (+0.21%) ⬆️
src/sage/schemes/elliptic_curves/cardinality.py 87.54% <100.00%> (+0.93%) ⬆️
... and 75 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Mar 1, 2024

Documentation preview for this PR (built with commit caead99; changes) is ready! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permutation(n) doesn't use the cache if there is one.

3 participants