Opened 4 years ago

Closed 4 years ago

#23912 closed enhancement (fixed)

parent() is slow for non-Elements

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: coercion Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b87032e (Commits, GitHub, GitLab) Commit: b87032e4aad9ce2064ac46ed365ff93c0c4b0e57
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The current implementation of the global function parent() is:

cpdef inline parent(x):
    if isinstance(x, Element):
        return (<Element>x)._parent
    # Fast check for "number" types, including int and float
    if PyNumber_Check(x):
        return type(x)
    try:
        p = x.parent
    except AttributeError:
        return type(x)
    else:
        return p()

It turns out that the non-Element case is sufficiently slow to cause an important slow-down, as observed in #23905.

There are a few classes in Sage which do not inherit from Element but still define a parent() method:

src/sage/algebras/cluster_algebra.py:class ClusterAlgebraSeed(SageObject)
src/sage/combinat/root_system/weyl_characters.py:class WeightRing(CombinatorialFreeModule)
src/sage/combinat/species/structure.py:class GenericSpeciesStructure(CombinatorialObject)
src/sage/combinat/words/abstract_word.py:class Word_class(SageObject)
src/sage/dynamics/flat_surfaces/strata.py:class ConnectedComponentOfAbelianStratum(SageObject)
src/sage/geometry/lattice_polytope.py:class LatticePolytopeClass(SageObject, collections.Hashable)
src/sage/schemes/elliptic_curves/heegner.py:class GaloisAutomorphism(SageObject)
src/sage/structure/sage_object.pyx:cdef class SageObject  # Returns type(self)

Change History (8)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/parent___is_slow_for_non_elements

comment:4 Changed 4 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to b87032e4aad9ce2064ac46ed365ff93c0c4b0e57
  • Milestone changed from sage-8.1 to sage-pending
  • Status changed from new to needs_review

New commits:

b87032eIn parent(x), do not call x.parent()

comment:5 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

In some of those cases, there are probably better names than parent but were initially chosen to have compatibility of sorts with Sage's framework. Anyways, I doubt it would cause a problem in any private code.

comment:6 Changed 4 years ago by jdemeyer

Note: I put this ticket in the milestone sage-pending to wait a few days to see how the discussion on sage-devel turns out.

comment:7 Changed 4 years ago by tscrim

  • Milestone changed from sage-pending to sage-8.1

I think we can set this back to be merged into Sage.

comment:8 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/parent___is_slow_for_non_elements to b87032e4aad9ce2064ac46ed365ff93c0c4b0e57
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.