Opened 5 years ago

Closed 5 years ago

#19416 closed enhancement (fixed)

Implement some generic category methods

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-6.10
Component: categories Keywords:
Cc: jhpalmieri Merged in:
Authors: Travis Scrimshaw Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: ff914dc (Commits) Commit: ff914dc988541148b4f10a3434dd688fc5ff7e75
Dependencies: Stopgaps:

Description

We implement a generic is_commutative test for finite-dimensional algebras with a basis and a cardinality for modules with a basis.

Change History (6)

comment:1 Changed 5 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/categories/generic_methods-19416
  • Commit set to f9b5da0e38fc793122ac2615df86efa3a590f3ad
  • Status changed from new to needs_review

New commits:

f9b5da0Implement generic cardinality() and is_commutative().

comment:2 follow-up: Changed 5 years ago by jhpalmieri

I would prefer if the docstring for is_commutative said "Return whether self is a commutative algebra" (or "Return whether this algebra is a commutative algebra").

Is it worth saving any time by removing self.one() from the basis before doing the iteration? With the following change:

  • src/sage/categories/finite_dimensional_algebras_with_basis.py

    diff --git a/src/sage/categories/finite_dimensional_algebras_with_basis.py b/src/sage/categories/finite_dimensional_algebras_with_basis.py
    index da1a870..8934e6c 100644
    a b class FiniteDimensionalAlgebrasWithBasis(CategoryWithAxiom_over_base_ring): 
    951951        @cached_method
    952952        def is_commutative(self):
    953953            """
    954             Return if ``self`` is a commutative algebra.
     954            Return whether ``self`` is a commutative algebra.
    955955
    956956            EXAMPLES::
    957957
    class FiniteDimensionalAlgebrasWithBasis(CategoryWithAxiom_over_base_ring): 
    963963                True
    964964            """
    965965            B = list(self.basis())
     966            B.remove(self.one())
    966967            return all(b*bp == bp*b for i,b in enumerate(B) for bp in B[i+1:])
    967968
    968969    class ElementMethods:

it appears to be faster to check commutativity for SymmetricGroupAlgebra(5, QQ). (I removed the @cached_method line before testing.) It's even a little faster for commutative algebras: the time spent removing the element from the list is made up by the time saved by not having as many iterations.

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

comment:3 Changed 5 years ago by git

  • Commit changed from f9b5da0e38fc793122ac2615df86efa3a590f3ad to ff914dc988541148b4f10a3434dd688fc5ff7e75

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

0c62b7cMerge branch 'develop' into public/categories/generic_methods-19416
ff914dcImplementing John's suggestions.

comment:4 in reply to: ↑ 2 Changed 5 years ago by tscrim

Replying to jhpalmieri:

I would prefer if the docstring for is_commutative said "Return whether self is a commutative algebra" (or "Return whether this algebra is a commutative algebra").

Done.

Is it worth saving any time by removing self.one() from the basis before doing the iteration? With the following change: ... it appears to be faster to check commutativity for SymmetricGroupAlgebra(5, QQ). (I removed the @cached_method line before testing.) It's even a little faster for commutative algebras: the time spent removing the element from the list is made up by the time saved by not having as many iterations.

The only possible issue I see is that not all algebras in Sage have one as a basis element, so for those algebras, by adding this extra remove check, it might be longer. However, since the vast majority of algebras in Sage do have one as a basis element, and probably it appears early on, this should be faster and I think it is worthwhile to have it.

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

comment:5 Changed 5 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Looks good, thanks.

comment:6 Changed 5 years ago by vbraun

  • Branch changed from public/categories/generic_methods-19416 to ff914dc988541148b4f10a3434dd688fc5ff7e75
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.