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:  sage9.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: 
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
Branch:  → u/mkoeppe/finiterankfreemodule_needs___classcall__ 

comment:2 followup: 11 Changed 2 years ago by
Authors:  → Matthias Koeppe 

Commit:  → 474b5399e3e00877a50f100682ddb02db067c515 
Status:  new → needs_review 
comment:3 Changed 2 years ago by
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
Commit:  474b5399e3e00877a50f100682ddb02db067c515 → 54770acb26f42a7e8420b875989d45a3e887d25e 

Branch pushed to git repo; I updated commit sha1. New commits:
54770ac  FiniteRankFreeModule.__init__: Restore default parameters

comment:5 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:6 Changed 2 years ago by
Commit:  54770acb26f42a7e8420b875989d45a3e887d25e → 8d9fbc8ea70afb135a63c8341965b3aaff9d8e69 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
55f7c63  TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Use category from fmodule

8d9fbc8  FiniteRankFreeModule: Add __classcall_private__, but also keep default args handling in __init__ for subclasses

comment:7 Changed 2 years ago by
Status:  needs_work → needs_review 

Let's see if the bot likes this version better
comment:8 Changed 2 years ago by
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
comment:9 followup: 10 Changed 2 years ago by
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 Changed 2 years ago by
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 aTensorFreeModule
.
Note that without "dual", the three are identified:
sage: M is M.tensor_module(1, 0) True
comment:11 Changed 2 years ago by
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.
comment:12 Changed 2 years ago by
Commit:  8d9fbc8ea70afb135a63c8341965b3aaff9d8e69 → c184ae119d37f091886b176bf37f8b2a11c352c8 

Branch pushed to git repo; I updated commit sha1. New commits:
c184ae1  TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Compute the categories

comment:13 Changed 2 years ago by
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
Commit:  c184ae119d37f091886b176bf37f8b2a11c352c8 → d38cd5042a7e57839e99c6acec1a8403a0110a13 

Branch pushed to git repo; I updated commit sha1. New commits:
d38cd50  ExtPowerFreeModule, ExtPowerDualFreeModule, TensorFreeModule: Use __classcall_private__ to delegate trivial cases

comment:15 Changed 2 years ago by
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:17 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:18 Changed 2 years ago by
Commit:  d38cd5042a7e57839e99c6acec1a8403a0110a13 → 6d6edba7345aba85ab96df4b72cdda659491e05c 

comment:19 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:20 followup: 24 Changed 2 years ago by
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 oneline descriptions should end in periods/fullstops.)
comment:21 Changed 2 years ago by
Commit:  6d6edba7345aba85ab96df4b72cdda659491e05c → 6993199aed5eed8d282e4725ada7deb7e5e55ca9 

Branch pushed to git repo; I updated commit sha1. New commits:
6993199  ExtPowerFreeModule: Make it a quotient of a TensorFreeModule

comment:22 Changed 2 years ago by
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
Commit:  6993199aed5eed8d282e4725ada7deb7e5e55ca9 → 0930a4abe8c3a04650134c8dfe30eae24a23f8bb 

Branch pushed to git repo; I updated commit sha1. New commits:
0930a4a  Add doctest for extra_super_categories

comment:24 followup: 29 Changed 2 years ago by
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
Commit:  0930a4abe8c3a04650134c8dfe30eae24a23f8bb → 7db43ed666921de730ec87104a76445584285307 

comment:26 Changed 2 years ago by
Dependencies:  → #30241 

comment:27 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:28 Changed 2 years ago by
Dependencies:  #30241 → #30241, #30242 

comment:29 Changed 2 years ago by
Replying to mkoeppe:
Replying to tscrim:
The
extra_super_categories
needs a doctestDone, 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:31 Changed 2 years ago by
Commit:  7db43ed666921de730ec87104a76445584285307 → 98c8df19a361fa66e3646e0f331e456f3270a50b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
98c8df1  FiniteRankFreeModule: Add __classcall_private__, but also keep default args handling in __init__ for subclasses

comment:32 Changed 2 years ago by
Dependencies:  #30241, #30242 

Status:  needs_work → 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 2 years ago by
Status:  needs_review → positive_review 

Okay, LGTM. (Assuming the patchbot will be green since it came back green before.)
comment:35 Changed 2 years ago by
Branch:  u/mkoeppe/finiterankfreemodule_needs___classcall__ → 98c8df19a361fa66e3646e0f331e456f3270a50b 

Resolution:  → fixed 
Status:  positive_review → closed 
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:
TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Use category from fmodule
FiniteRankFreeModule: Add __classcall_private__