Opened 11 months ago
Closed 11 months ago
#30169 closed defect (fixed)
FiniteRankFreeModule needs __classcall__
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  linear algebra  Keywords:  
Cc:  egourgoulhon, tscrim, ghmjungmath  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 11 months ago by
 Branch set to u/mkoeppe/finiterankfreemodule_needs___classcall__
comment:2 followup: ↓ 11 Changed 11 months ago by
 Commit set to 474b5399e3e00877a50f100682ddb02db067c515
 Status changed from new to needs_review
comment:3 Changed 11 months 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 11 months ago by
 Commit changed from 474b5399e3e00877a50f100682ddb02db067c515 to 54770acb26f42a7e8420b875989d45a3e887d25e
Branch pushed to git repo; I updated commit sha1. New commits:
54770ac  FiniteRankFreeModule.__init__: Restore default parameters

comment:5 Changed 11 months ago by
 Status changed from needs_review to needs_work
comment:6 Changed 11 months ago by
 Commit changed from 54770acb26f42a7e8420b875989d45a3e887d25e to 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 11 months ago by
 Status changed from needs_work to needs_review
Let's see if the bot likes this version better
comment:8 Changed 11 months 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 11 months ago by
 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 11 months 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 in reply to: ↑ 2 Changed 11 months 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 11 months ago by
 Commit changed from 8d9fbc8ea70afb135a63c8341965b3aaff9d8e69 to c184ae119d37f091886b176bf37f8b2a11c352c8
Branch pushed to git repo; I updated commit sha1. New commits:
c184ae1  TensorFreeModule, ExtPowerFreeModule, ExtPowerDualFreeModule: Compute the categories

comment:13 Changed 11 months 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 11 months ago by
 Commit changed from c184ae119d37f091886b176bf37f8b2a11c352c8 to 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 11 months 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:16 Changed 11 months ago by
Thank you! I'll work on this
comment:17 Changed 11 months ago by
 Status changed from needs_review to needs_work
comment:18 Changed 11 months ago by
 Commit changed from d38cd5042a7e57839e99c6acec1a8403a0110a13 to 6d6edba7345aba85ab96df4b72cdda659491e05c
comment:19 Changed 11 months ago by
 Status changed from needs_work to needs_review
comment:20 followup: ↓ 24 Changed 11 months 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 11 months ago by
 Commit changed from 6d6edba7345aba85ab96df4b72cdda659491e05c to 6993199aed5eed8d282e4725ada7deb7e5e55ca9
Branch pushed to git repo; I updated commit sha1. New commits:
6993199  ExtPowerFreeModule: Make it a quotient of a TensorFreeModule

comment:22 Changed 11 months 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 11 months ago by
 Commit changed from 6993199aed5eed8d282e4725ada7deb7e5e55ca9 to 0930a4abe8c3a04650134c8dfe30eae24a23f8bb
Branch pushed to git repo; I updated commit sha1. New commits:
0930a4a  Add doctest for extra_super_categories

comment:24 in reply to: ↑ 20 ; followup: ↓ 29 Changed 11 months 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 11 months ago by
 Commit changed from 0930a4abe8c3a04650134c8dfe30eae24a23f8bb to 7db43ed666921de730ec87104a76445584285307
comment:26 Changed 11 months ago by
 Dependencies set to #30241
comment:27 Changed 11 months ago by
 Status changed from needs_review to needs_work
comment:28 Changed 11 months ago by
 Dependencies changed from #30241 to #30241, #30242
comment:29 in reply to: ↑ 24 Changed 11 months 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:30 Changed 11 months ago by
I'll work on this in #30252.
comment:31 Changed 11 months ago by
 Commit changed from 7db43ed666921de730ec87104a76445584285307 to 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 11 months ago by
 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 11 months ago by
 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 11 months ago by
Thanks!
comment:35 Changed 11 months ago by
 Branch changed from u/mkoeppe/finiterankfreemodule_needs___classcall__ to 98c8df19a361fa66e3646e0f331e456f3270a50b
 Resolution set to fixed
 Status changed from positive_review to 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__