Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common base class for FiniteRankFreeModule and TensorFreeModule #34424

Closed
mkoeppe opened this issue Aug 24, 2022 · 22 comments
Closed

Common base class for FiniteRankFreeModule and TensorFreeModule #34424

mkoeppe opened this issue Aug 24, 2022 · 22 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 24, 2022

Currently TensorFreeModule (and similar classes) inherits many methods from FiniteRankFreeModule that actually only work with the base module.
Trying to use them leads to incomprehensible error messages. For example:

sage: M = FiniteRankFreeModule(ZZ, 3, name='M')
sage: M.tensor_module(2, 1).tensor_module(3, 3)
Free module of type-(3,3) tensors on the Free module of type-(2,1) tensors on the Rank-3 free module M over the Integer Ring
sage: _.an_element()
ValueError: the None has not been defined on the Free module of type-(2,1) tensors on the Rank-3 free module M over the Integer Ring

As discussed in #30229 comment:6, we introduce a common base class that only defines the methods that should be inherited by TensorFreeModule and other such classes.

Depends on #31276

CC: @egourgoulhon @tscrim

Component: linear algebra

Author: Matthias Koeppe

Branch/Commit: 451ebe5

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/34424

@mkoeppe mkoeppe added this to the sage-9.7 milestone Aug 24, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 24, 2022

comment:1

I'd just call it FiniteRankFreeModule_abstract in analogy to the existing Basis_abstract unless there are better suggestions

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

comment:3

Here's a basic version that works.

Some more of the methods can probably be moved to FiniteRankFreeModule_abstract -- but currently all doctests pass as is, so in this case we should add some examples that use subclasses.


New commits:

e76ee2fFiniteRankFreeModule_abstract: New common base class for FiniteRankFreeModule, TensorFreeModule, ExtPower*FreeModule

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

Commit: e76ee2f

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2022

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

Changed commit from e76ee2f to ac6d465

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

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

ac6d465FiniteRankFreeModule_abstract: No need for _sindex

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

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

7e3ae02FiniteRankFreeModule_abstract: No need for _output_formatter

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2022

Changed commit from ac6d465 to 7e3ae02

@egourgoulhon
Copy link
Member

comment:7

Thanks for implementing this!

Replying to @mkoeppe:

I'd just call it FiniteRankFreeModule_abstract in analogy to the existing Basis_abstract unless there are better suggestions

Sounds good to me.

@egourgoulhon
Copy link
Member

comment:8

Just one suggestion: in the docstring of FiniteRankFreeModule_abstract:

- Free module of finite rank over a commutative ring.
+ Abstract base class for free modules of finite rank over a commutative ring.

Otherwise, LGTM.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2022

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

134c44fsrc/sage/tensor/modules/finite_rank_free_module.py: Better docstring for FiniteRankFreeModule_abstract

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2022

Changed commit from 7e3ae02 to 134c44f

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:10

Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2022

comment:11

Thank you!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 28, 2022

Dependencies: #31276

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 28, 2022

comment:12

When merged with #31276, this gives a doctest failure

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2022

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

1ea94e1FiniteRankFreeModule.tensor_type: New
0fe1f0dFiniteRankFreeModule.tensor_product: New
3929d2eFiniteRankFreeModule.tensor_power: New
bc1dc24Merge #31276
451ebe5src/sage/tensor/modules/finite_rank_free_module.py (FiniteRankFreeModule_abstract): Move tensor_power, tensor_product here from FiniteRankFreeModule

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2022

Changed commit from 134c44f to 451ebe5

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 29, 2022

comment:14

Bots are green, back to positive review (hoping that's OK)

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

@vbraun vbraun closed this as completed in 523695c Aug 30, 2022
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
…on a FiniteRankFreeModule

After sagemath#34424 (merged in Sage 9.7.rc0), the documentation of
`TensorFreeModule`, `ExtPowerFreeModule` and `ExtPowerDualFreeModule`
should say that these classes inherit from
`FiniteRankFreeModule_abstract`, not `FiniteRankFreeModule`.

URL: https://trac.sagemath.org/34792
Reported by: egourgoulhon
Ticket author(s): Eric Gourgoulhon
Reviewer(s): Matthias Koeppe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants