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: |
Description (last modified by )
The method IndexedFreeModuleElement._coefficient_fast
serves no purpose at all:
- It actually makes
__getitem__
slower because of the extra indirection.
- Special methods like
__getitem__
in extension classes are very fast already, as fast as Cythoncdef
functions.
- When called from Python (not Cython), a special method is much faster than a method.
- 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
- Description modified (diff)
comment:2 Changed 3 years ago by
- Description modified (diff)
comment:3 Changed 3 years ago by
- Branch set to u/jdemeyer/remove_indexedfreemoduleelement__coefficient_fast
comment:4 Changed 3 years ago by
- Commit set to 1c36c1409b7acf7056b42ef9a9f11cce3bad6ce9
- Description modified (diff)
- Status changed from new to needs_review
comment:5 Changed 3 years ago by
- Description modified (diff)
comment:6 follow-up: ↓ 7 Changed 3 years ago by
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: ↓ 8 Changed 3 years ago by
Replying to tscrim:
Everything is fine with me except for removing the
__getitem__
fromModulesWithBasis
. It is used by the elements ofDifferentialWeylAlgebra
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
Replying to jdemeyer:
Replying to tscrim:
Everything is fine with me except for removing the
__getitem__
fromModulesWithBasis
. It is used by the elements ofDifferentialWeylAlgebra
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
- Commit changed from 1c36c1409b7acf7056b42ef9a9f11cce3bad6ce9 to e4e1434f5682b10248c914d10a697f668694df77
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e4e1434 | Remove IndexedFreeModuleElement._coefficient_fast
|
comment:10 Changed 3 years ago by
Done, needs review again.
comment:11 Changed 3 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:12 Changed 3 years ago by
- Branch changed from u/jdemeyer/remove_indexedfreemoduleelement__coefficient_fast to e4e1434f5682b10248c914d10a697f668694df77
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Remove IndexedFreeModuleElement._coefficient_fast