Opened 2 years ago

Closed 2 years ago

#30169 closed defect (fixed)

FiniteRankFreeModule needs __classcall__

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.2
Component: linear algebra Keywords:
Cc: Eric Gourgoulhon, Travis Scrimshaw, Michael Jung Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 98c8df1 (Commits, GitHub, GitLab) Commit: 98c8df19a361fa66e3646e0f331e456f3270a50b
Dependencies: Stopgaps:

Status badges

Description

... to normalize init arguments for UniqueRepresentation.

sage: FiniteRankFreeModule(QQ, 3) is FiniteRankFreeModule(QQ, 3)
True
sage: FiniteRankFreeModule(QQ, 3) is FiniteRankFreeModule(QQ, 3, name=None)
False

Change History (35)

comment:1 Changed 2 years ago by Matthias Köppe

Branch: u/mkoeppe/finiterankfreemodule_needs___classcall__

comment:2 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe
Commit: 474b5399e3e00877a50f100682ddb02db067c515
Status: newneeds_review

I am not sure about the change I made in the first commit. Surely the category should depend on the category of the underlying free module, but I suspect some construction functor needs to come into play.


New commits:

ca2ce4aTensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Use category from fmodule
474b539FiniteRankFreeModule: Add __classcall_private__

comment:3 Changed 2 years ago by Travis Scrimshaw

You are getting a lot of doctest failures because the FiniteRankFreeModule.__init__ now explicitly requires all 7 arguments from its subclasses. So I would keep the default parameters.

comment:4 Changed 2 years ago by git

Commit: 474b5399e3e00877a50f100682ddb02db067c51554770acb26f42a7e8420b875989d45a3e887d25e

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

54770acFiniteRankFreeModule.__init__: Restore default parameters

comment:5 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:6 Changed 2 years ago by git

Commit: 54770acb26f42a7e8420b875989d45a3e887d25e8d9fbc8ea70afb135a63c8341965b3aaff9d8e69

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

55f7c63TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Use category from fmodule
8d9fbc8FiniteRankFreeModule: Add __classcall_private__, but also keep default args handling in __init__ for subclasses

comment:7 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

Let's see if the bot likes this version better

comment:8 Changed 2 years ago by Matthias Köppe

There are also some other unique representation issues in these classes.

sage: M = FiniteRankFreeModule(ZZ, 3, name='M')
sage: M is M.exterior_power(1)
True
sage: M.dual() is M.dual_exterior_power(1)
True
sage: M.dual() is M.tensor_module(0, 1)
False

If I'm not mistaken, they should all be the same

Last edited 2 years ago by Matthias Köppe (previous) (diff)

comment:9 Changed 2 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw

Since this fixes the problem with the ticket description, I would be willing to set a positive review. However, I suspect there might be some test failures given the latest working patchbot report. So I want to wait for a working patchbot report.

I am not convinced they should be the same because something in the tensor space should not be the same thing, even though they are isomorphic. The user might expect certain features and semantics that would be different if M.tensor_module(0, 1) does not return a TensorFreeModule.

comment:10 in reply to:  9 Changed 2 years ago by Matthias Köppe

Replying to tscrim:

I am not convinced they should be the same because something in the tensor space should not be the same thing, even though they are isomorphic. The user might expect certain features and semantics that would be different if M.tensor_module(0, 1) does not return a TensorFreeModule.

Note that without "dual", the three are identified:

sage: M is M.tensor_module(1, 0)
True

comment:11 in reply to:  2 Changed 2 years ago by Matthias Köppe

Replying to mkoeppe:

I am not sure about the change I made in the first commit. Surely the category should depend on the category of the underlying free module, but I suspect some construction functor needs to come into play.

Working on a version that improves this bit.

Last edited 2 years ago by Matthias Köppe (previous) (diff)

comment:12 Changed 2 years ago by git

Commit: 8d9fbc8ea70afb135a63c8341965b3aaff9d8e69c184ae119d37f091886b176bf37f8b2a11c352c8

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

c184ae1TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Compute the categories

comment:13 Changed 2 years ago by Matthias Köppe

Here I am running into the following problem TensorProducts seems to forget finite dimension...

sage: Modules(ZZ).FiniteDimensional().TensorProducts()
Category of tensor products of modules over Integer Ring
sage: Modules(ZZ).FiniteDimensional().TensorProducts().FiniteDimensional()
Category of finite dimensional tensor products of modules over Integer Ring

How to fix?

comment:14 Changed 2 years ago by git

Commit: c184ae119d37f091886b176bf37f8b2a11c352c8d38cd5042a7e57839e99c6acec1a8403a0110a13

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

d38cd50ExtPowerFreeModule, ExtPowerDualFreeModule, TensorFreeModule: Use __classcall_private__ to delegate trivial cases

comment:15 Changed 2 years ago by Travis Scrimshaw

The TensorProducts category needs to know it is finite dimensional (axioms do not naturally commute with functorial constructions; as they should not), so it needs a

    class TensorProducts(TensorProductsCategory):
        def extra_super_categories(self):
            return [self.base_category()]

See modules_with_basis.py.

comment:16 Changed 2 years ago by Matthias Köppe

Thank you! I'll work on this

comment:17 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:18 Changed 2 years ago by git

Commit: d38cd5042a7e57839e99c6acec1a8403a0110a136d6edba7345aba85ab96df4b72cdda659491e05c

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

b3c9a0fModules: Make TensorProducts of finite-dimensional modules finite-dimensional
6d6edbaExtPowerFreeModule, ExtPowerDualFreeModule: Disable subquotient category for now

comment:19 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:20 Changed 2 years ago by Travis Scrimshaw

The extra_super_categories needs a doctest, but other than that (and a green patchbot), LGTM. (If I really wanted to haggle, I would say the one-line descriptions should end in periods/full-stops.)

comment:21 Changed 2 years ago by git

Commit: 6d6edba7345aba85ab96df4b72cdda659491e05c6993199aed5eed8d282e4725ada7deb7e5e55ca9

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

6993199ExtPowerFreeModule: Make it a quotient of a TensorFreeModule

comment:22 Changed 2 years ago by Matthias Köppe

Getting closer to getting the categories right. ExtPowerFreeModule is now a quotient. What's missing next is DualObjects of a module

comment:23 Changed 2 years ago by git

Commit: 6993199aed5eed8d282e4725ada7deb7e5e55ca90930a4abe8c3a04650134c8dfe30eae24a23f8bb

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

0930a4aAdd doctest for extra_super_categories

comment:24 in reply to:  20 ; Changed 2 years ago by Matthias Köppe

Replying to tscrim:

The extra_super_categories needs a doctest

Done, but I noticed that it fails for QQ:

   sage: Modules(QQ).FiniteDimensional().TensorProducts().extra_super_categories()
   AttributeError: 'JoinCategory_with_category' object has no attribute 'extra_super_categories'

Do I need to duplicate all of this for VectorSpaces?

comment:25 Changed 2 years ago by git

Commit: 0930a4abe8c3a04650134c8dfe30eae24a23f8bb7db43ed666921de730ec87104a76445584285307

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

07e0346Add some full stops in docstrings
7db43edRemove completed TODO item

comment:26 Changed 2 years ago by Matthias Köppe

Dependencies: #30241

comment:27 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:28 Changed 2 years ago by Matthias Köppe

Dependencies: #30241#30241, #30242

comment:29 in reply to:  24 Changed 2 years ago by Travis Scrimshaw

Replying to mkoeppe:

Replying to tscrim:

The extra_super_categories needs a doctest

Done, but I noticed that it fails for QQ:

   sage: Modules(QQ).FiniteDimensional().TensorProducts().extra_super_categories()
   AttributeError: 'JoinCategory_with_category' object has no attribute 'extra_super_categories'

Do I need to duplicate all of this for VectorSpaces?

Unfortunately...

comment:30 Changed 2 years ago by Matthias Köppe

I'll work on this in #30252.

comment:31 Changed 2 years ago by git

Commit: 7db43ed666921de730ec87104a7644558428530798c8df19a361fa66e3646e0f331e456f3270a50b

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

98c8df1FiniteRankFreeModule: Add __classcall_private__, but also keep default args handling in __init__ for subclasses

comment:32 Changed 2 years ago by Matthias Köppe

Dependencies: #30241, #30242
Status: needs_workneeds_review

I've reduced to the branch to take care only of what is promised in the ticket description. I'll take care of the other things discussed in independent tickets.

comment:33 Changed 2 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Okay, LGTM. (Assuming the patchbot will be green since it came back green before.)

comment:34 Changed 2 years ago by Matthias Köppe

Thanks!

comment:35 Changed 2 years ago by Volker Braun

Branch: u/mkoeppe/finiterankfreemodule_needs___classcall__98c8df19a361fa66e3646e0f331e456f3270a50b
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.