Opened 3 months ago

Closed 3 months ago

#30169 closed defect (fixed)

FiniteRankFreeModule needs __classcall__

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: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 98c8df1 (Commits) Commit: 98c8df19a361fa66e3646e0f331e456f3270a50b
Dependencies: Stopgaps:

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

  • Branch set to u/mkoeppe/finiterankfreemodule_needs___classcall__

comment:2 follow-up: Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 474b5399e3e00877a50f100682ddb02db067c515
  • Status changed from new to needs_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 3 months ago by tscrim

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

  • Commit changed from 474b5399e3e00877a50f100682ddb02db067c515 to 54770acb26f42a7e8420b875989d45a3e887d25e

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

54770acFiniteRankFreeModule.__init__: Restore default parameters

comment:5 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:6 Changed 3 months ago by git

  • Commit changed from 54770acb26f42a7e8420b875989d45a3e887d25e to 8d9fbc8ea70afb135a63c8341965b3aaff9d8e69

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

  • Status changed from needs_work to needs_review

Let's see if the bot likes this version better

comment:8 Changed 3 months ago by mkoeppe

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 3 months ago by mkoeppe (previous) (diff)

comment:9 follow-up: Changed 3 months ago by tscrim

  • Reviewers set to 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 3 months ago by mkoeppe

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

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 3 months ago by mkoeppe (previous) (diff)

comment:12 Changed 3 months ago by git

  • Commit changed from 8d9fbc8ea70afb135a63c8341965b3aaff9d8e69 to c184ae119d37f091886b176bf37f8b2a11c352c8

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

c184ae1TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Compute the categories

comment:13 Changed 3 months ago by mkoeppe

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

  • Commit changed from c184ae119d37f091886b176bf37f8b2a11c352c8 to d38cd5042a7e57839e99c6acec1a8403a0110a13

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

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

comment:15 Changed 3 months ago by tscrim

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

Thank you! I'll work on this

comment:17 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:18 Changed 3 months ago by git

  • Commit changed from d38cd5042a7e57839e99c6acec1a8403a0110a13 to 6d6edba7345aba85ab96df4b72cdda659491e05c

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

  • Status changed from needs_work to needs_review

comment:20 follow-up: Changed 3 months ago by tscrim

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

  • Commit changed from 6d6edba7345aba85ab96df4b72cdda659491e05c to 6993199aed5eed8d282e4725ada7deb7e5e55ca9

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

6993199ExtPowerFreeModule: Make it a quotient of a TensorFreeModule

comment:22 Changed 3 months ago by mkoeppe

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

comment:23 Changed 3 months ago by git

  • Commit changed from 6993199aed5eed8d282e4725ada7deb7e5e55ca9 to 0930a4abe8c3a04650134c8dfe30eae24a23f8bb

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

0930a4aAdd doctest for extra_super_categories

comment:24 in reply to: ↑ 20 ; follow-up: Changed 3 months ago by 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?

comment:25 Changed 3 months ago by git

  • Commit changed from 0930a4abe8c3a04650134c8dfe30eae24a23f8bb to 7db43ed666921de730ec87104a76445584285307

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

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

comment:26 Changed 3 months ago by mkoeppe

  • Dependencies set to #30241

comment:27 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:28 Changed 3 months ago by mkoeppe

  • Dependencies changed from #30241 to #30241, #30242

comment:29 in reply to: ↑ 24 Changed 3 months ago by tscrim

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

I'll work on this in #30252.

comment:31 Changed 3 months ago by git

  • Commit changed from 7db43ed666921de730ec87104a76445584285307 to 98c8df19a361fa66e3646e0f331e456f3270a50b

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

  • Dependencies #30241, #30242 deleted
  • Status changed from needs_work to needs_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 3 months ago by tscrim

  • Status changed from needs_review to positive_review

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

comment:34 Changed 3 months ago by mkoeppe

Thanks!

comment:35 Changed 3 months ago by vbraun

  • Branch changed from u/mkoeppe/finiterankfreemodule_needs___classcall__ to 98c8df19a361fa66e3646e0f331e456f3270a50b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.