Opened 3 years ago

Closed 3 years ago

#27097 closed enhancement (fixed)

Remove IndexedFreeModuleElement._coefficient_fast

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

Status badges

Description (last modified by jdemeyer)

The method IndexedFreeModuleElement._coefficient_fast serves no purpose at all:

  1. It actually makes __getitem__ slower because of the extra indirection.
  1. Special methods like __getitem__ in extension classes are very fast already, as fast as Cython cdef functions.
  1. When called from Python (not Cython), a special method is much faster than a method.
  1. It is called in only one place (namely __getitem__), so even if it would be faster, its usage is dubious.

Since this is a private underscored method, we can just remove it without deprecation.

Furthermore, the implementation of __getitem__ can be improved by only computing self.base_ring().zero() if needed.

Finally, in the category ModulesWithBasis, I am also removing ElementMethods.__getitem__ because it doesn't seem to be used anywhere (to be checked by the patchbot).

Change History (12)

comment:1 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/remove_indexedfreemoduleelement__coefficient_fast

comment:4 Changed 3 years ago by jdemeyer

  • Commit set to 1c36c1409b7acf7056b42ef9a9f11cce3bad6ce9
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

​1c36c14Remove IndexedFreeModuleElement._coefficient_fast

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:6 follow-up: Changed 3 years ago by tscrim

Everything is fine with me except for removing the __getitem__ from ModulesWithBasis. It is used by the elements of DifferentialWeylAlgebra (although it is not practically so useful because of the index set of the basis). Moreover, it is something that could be used in the wild by something in ModulesWithBasis that is not a subclass of IndexedFreeModuleElement. So -1 on removing it.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by jdemeyer

Replying to tscrim:

Everything is fine with me except for removing the __getitem__ from ModulesWithBasis. It is used by the elements of DifferentialWeylAlgebra

Removing it doesn't cause any doctest failures (see patchbot). If we keep it, there should at least be one doctest that relies on it (i.e. that would break if we remove this). Could you come up with such a test?

comment:8 in reply to: ↑ 7 Changed 3 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

Everything is fine with me except for removing the __getitem__ from ModulesWithBasis. It is used by the elements of DifferentialWeylAlgebra

Removing it doesn't cause any doctest failures (see patchbot). If we keep it, there should at least be one doctest that relies on it (i.e. that would break if we remove this). Could you come up with such a test?

This should do it: 

sage: W.<x,y,z> = DifferentialWeylAlgebra(QQ)
sage: x[((1,0,0),(0,0,0))]
1

comment:9 Changed 3 years ago by git

  • Commit changed from 1c36c1409b7acf7056b42ef9a9f11cce3bad6ce9 to e4e1434f5682b10248c914d10a697f668694df77

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

​e4e1434Remove IndexedFreeModuleElement._coefficient_fast

comment:10 Changed 3 years ago by jdemeyer

Done, needs review again.

comment:11 Changed 3 years ago by tscrim

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

Thank you. LGTM.

comment:12 Changed 3 years ago by vbraun

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