Opened 3 months ago

Closed 3 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) 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 3 months ago by mkoeppe

  • Dependencies set to #30254

comment:2 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/finiterankfreemodule__move_all_module_identifications_to_methods_exterior_power__dual_exterior_power__tensor_module

comment:3 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 2ac26d7dcf82bebe56296623376b1462e60b206d
  • Dependencies changed from #30254 to #30250, #30254
  • Status changed from new to needs_review

New commits:

1bd50f8FiniteRankFreeModule: Move all module identifications to methods exterior_power, dual_exterior_power, tensor_module; Rewrite in try/except style
d64c8edExtPowerFreeModule._an_element_: Make sure a default basis is available
d2d7f42ExtPowerDualFreeModule._an_element_: Make sure a default basis is available
9e739e5FreeModuleLinearGroup._an_element_: Make sure a default basis is available
eec19fdTensorFreeModule._an_element_: Make sure a default basis is available
1378d06Merge 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
5a340d9TensorFreeModule.__init__: Remove duplicate registration of self in the base module, uniqueness check
de35db3FreeModuleBasis.__init__: Update all modules in the set fmodule._all_modules
2ac26d7Merge 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

comment:4 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:5 Changed 3 months ago by mkoeppe

Ha! This one did not pass the sage.manifolds doctests. Hold on...

comment:6 Changed 3 months ago by git

  • Commit changed from 2ac26d7dcf82bebe56296623376b1462e60b206d to a6961c6f57e9021d1ccab54b9cbaeaf6ba581f0f

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

a6961c6VectorFieldFreeModule: Update methods exterior_power, dual_exterior_power, tensor_module for changed FiniteRankFreeModule.__init__

comment:7 Changed 3 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:8 Changed 3 months ago by mkoeppe

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 3 months ago by egourgoulhon

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: Changed 3 months ago by mkoeppe

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 3 months ago by tscrim

How does it compare in the slow path?

comment:12 Changed 3 months ago by mkoeppe

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 3 months ago by egourgoulhon

Replying to mkoeppe:

It's more than twice as fast in the fast path.

Thanks for the explanation.

comment:14 Changed 3 months ago by mkoeppe

Ready for review

comment:15 Changed 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath

  • Reviewers set to Michael Jung
  • Status changed from needs_review to positive_review

No okay, that's all. LGTM.

comment:17 Changed 3 months ago by gh-mjungmath

  • Status changed from positive_review to needs_review

Sorry. I'll give positive review when the line is removed.

comment:18 Changed 3 months ago by git

  • Commit changed from a6961c6f57e9021d1ccab54b9cbaeaf6ba581f0f to c035d06db85082324204d83f05236859b55450be

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

c035d06Remove unused import in doctest

comment:19 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

LGTM. Patchbot is now full green, too.

comment:20 Changed 3 months ago by mkoeppe

Thanks!

comment:21 Changed 3 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.