Opened 7 months ago
Closed 7 months ago
#30255 closed enhancement (fixed)
FiniteRankFreeModule: Move all module identifications to methods exterior_power, dual_exterior_power, tensor_module
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.2 |
Component: | linear algebra | Keywords: | |
Cc: | egourgoulhon, tscrim, gh-mjungmath | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | Michael Jung |
Report Upstream: | N/A | Work issues: | |
Branch: | c035d06 (Commits, GitHub, GitLab) | Commit: | c035d06db85082324204d83f05236859b55450be |
Dependencies: | #30250, #30254 | Stopgaps: |
Description
Currently, the identification of a base module M
with M.tensor_module(1, 0)
and M.exterior_power(1)
is implemented in FiniteRankFreeModule.__init__
but the identification of M.base_ring()
with M.exterior_power(0)
is implemented in exterior_power
.
In this ticket, we move all identifications to the methods tensor_module
, exterior_power
, dual_exterior_power
.
We also rewrite these methods in try/except style.
Change History (21)
comment:1 Changed 7 months ago by
- Dependencies set to #30254
comment:2 Changed 7 months ago by
- Branch set to u/mkoeppe/finiterankfreemodule__move_all_module_identifications_to_methods_exterior_power__dual_exterior_power__tensor_module
comment:3 Changed 7 months ago by
- Commit set to 2ac26d7dcf82bebe56296623376b1462e60b206d
- Dependencies changed from #30254 to #30250, #30254
- Status changed from new to needs_review
comment:4 Changed 7 months ago by
- Status changed from needs_review to needs_work
comment:5 Changed 7 months ago by
Ha! This one did not pass the sage.manifolds
doctests. Hold on...
comment:6 Changed 7 months ago by
- Commit changed from 2ac26d7dcf82bebe56296623376b1462e60b206d to a6961c6f57e9021d1ccab54b9cbaeaf6ba581f0f
Branch pushed to git repo; I updated commit sha1. New commits:
a6961c6 | VectorFieldFreeModule: Update methods exterior_power, dual_exterior_power, tensor_module for changed FiniteRankFreeModule.__init__
|
comment:7 Changed 7 months ago by
- Status changed from needs_work to needs_review
comment:8 Changed 7 months ago by
The code duplication between these three methods of FiniteRankFreeModule
and VectorFieldFreeModule
could of course be removed by using class attributes _exterior_power_class
, _exterior_dual_power_class
, _tensor_module_class
. I can do that on a follow-up ticket if you like.
comment:9 Changed 7 months ago by
Thanks for this cleaning.
What is the advantage of
try: return d[k] except KeyError: d[k] = ... return d[k]
over
if k not in d: d[k] = ... return d[k]
? The latter looks easier to read.
comment:10 follow-up: ↓ 13 Changed 7 months ago by
It's more than twice as fast in the fast path.
before:
sage: sage: M = FiniteRankFreeModule(ZZ, 3, name='M') sage: sage: M.exterior_power(2) 2nd exterior power of the Rank-3 free module M over the Integer Ring sage: %timeit M.exterior_power(2) The slowest run took 9.54 times longer than the fastest. This could mean that an intermediate result is being cached. 100000 loops, best of 5: 2.23 µs per loop
after:
sage: sage: M = FiniteRankFreeModule(ZZ, 3, name='M') sage: sage: M.exterior_power(2) 2nd exterior power of the Rank-3 free module M over the Integer Ring sage: %timeit M.exterior_power(2) The slowest run took 13.09 times longer than the fastest. This could mean that an intermediate result is being cached. 1000000 loops, best of 5: 929 ns per loop
comment:11 Changed 7 months ago by
How does it compare in the slow path?
comment:12 Changed 7 months ago by
The slow path is 2 orders of magnitude slower than the fast path because a heavy weight object is created:
sage: M = FiniteRankFreeModule(ZZ, 3, name='M') sage: %time M.exterior_power(1) CPU times: user 729 µs, sys: 692 µs, total: 1.42 ms Wall time: 891 µs Rank-3 free module M over the Integer Ring sage: %time M.exterior_power(2) CPU times: user 685 µs, sys: 18 µs, total: 703 µs Wall time: 707 µs 2nd exterior power of the Rank-3 free module M over the Integer Ring sage: %time M.exterior_power(3) CPU times: user 239 µs, sys: 11 µs, total: 250 µs Wall time: 254 µs 3rd exterior power of the Rank-3 free module M over the Integer Ring sage: %time M.exterior_power(4) CPU times: user 229 µs, sys: 0 ns, total: 229 µs Wall time: 233 µs 4th exterior power of the Rank-3 free module M over the Integer Ring sage: %time M.exterior_power(5) CPU times: user 269 µs, sys: 14 µs, total: 283 µs Wall time: 303 µs 5th exterior power of the Rank-3 free module M over the Integer Ring sage: %time M.exterior_power(0) CPU times: user 15 µs, sys: 1 µs, total: 16 µs Wall time: 16.7 µs Integer Ring
So whatever the changes to this method do does not matter for the slow path.
comment:13 in reply to: ↑ 10 Changed 7 months ago by
comment:14 Changed 7 months ago by
Ready for review
comment:15 Changed 7 months ago by
Very nice changes.
First, the import in the doctest is obsolete now:
- sage: from sage.tensor.modules.tensor_free_module import TensorFreeModule
More comments follow...
comment:16 Changed 7 months ago by
- Reviewers set to Michael Jung
- Status changed from needs_review to positive_review
No okay, that's all. LGTM.
comment:17 Changed 7 months ago by
- Status changed from positive_review to needs_review
Sorry. I'll give positive review when the line is removed.
comment:18 Changed 7 months ago by
- Commit changed from a6961c6f57e9021d1ccab54b9cbaeaf6ba581f0f to c035d06db85082324204d83f05236859b55450be
Branch pushed to git repo; I updated commit sha1. New commits:
c035d06 | Remove unused import in doctest
|
comment:19 Changed 7 months ago by
- Status changed from needs_review to positive_review
LGTM. Patchbot is now full green, too.
comment:20 Changed 7 months ago by
Thanks!
comment:21 Changed 7 months ago by
- Branch changed from u/mkoeppe/finiterankfreemodule__move_all_module_identifications_to_methods_exterior_power__dual_exterior_power__tensor_module to c035d06db85082324204d83f05236859b55450be
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
FiniteRankFreeModule: Move all module identifications to methods exterior_power, dual_exterior_power, tensor_module; Rewrite in try/except style
ExtPowerFreeModule._an_element_: Make sure a default basis is available
ExtPowerDualFreeModule._an_element_: Make sure a default basis is available
FreeModuleLinearGroup._an_element_: Make sure a default basis is available
TensorFreeModule._an_element_: Make sure a default basis is available
Merge branch 't/30254/tensorfreemodule__an_element___create_a_default_basis_in_the_base_module_if_necessary' into t/30255/finiterankfreemodule__move_all_module_identifications_to_methods_exterior_power__dual_exterior_power__tensor_module
TensorFreeModule.__init__: Remove duplicate registration of self in the base module, uniqueness check
FreeModuleBasis.__init__: Update all modules in the set fmodule._all_modules
Merge branch 't/30250/finiterankfreemodule__simplify_unique_representation_code_for_dependent_modules' into t/30255/finiterankfreemodule__move_all_module_identifications_to_methods_exterior_power__dual_exterior_power__tensor_module