Opened 9 months ago
Closed 3 months ago
#33431 closed enhancement (fixed)
Category of submodules of sage.modules.free_module.FreeModule_ambient_pid
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.8 
Component:  linear algebra  Keywords:  
Cc:  Travis Scrimshaw, ghlouisng114  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  62f28a3 (Commits, GitHub, GitLab)  Commit:  62f28a334262b6b6712c0a6d98e07291abcba5e2 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: (QQ^2).submodule([[1,1]]) Vector space of degree 2 and dimension 1 over Rational Field Basis matrix: [1 1] sage: _.category() Category of finite dimensional vector spaces with basis over (number fields and quotient fields and metric spaces)
Subobjects
is missing, so the standard methods ambient
, lift
, retract
are missing.
This is in contrast to CombinatorialFreeModule
.
(There is a classspecific method ambient_module
though.)
Also there is an apparent conflict of the element method lift
with the one defined in free_module_element.pyx
; not resolved here.
Change History (19)
comment:1 Changed 8 months ago by
Milestone:  sage9.6 → sage9.7 

comment:2 Changed 3 months ago by
Milestone:  sage9.7 → sage9.8 

comment:3 Changed 3 months ago by
Branch:  → u/mkoeppe/category_of_submodules_of_sage_modules_free_module_freemodule_ambient_pid 

comment:4 Changed 3 months ago by
Commit:  → 88c67d9704e6100b9585336802e2a746e7c15104 

comment:5 Changed 3 months ago by
Authors:  → Matthias Koeppe 

Status:  new → needs_review 
comment:6 Changed 3 months ago by
Description:  modified (diff) 

comment:7 Changed 3 months ago by
Description:  modified (diff) 

comment:8 Changed 3 months ago by
Commit:  88c67d9704e6100b9585336802e2a746e7c15104 → 823073652e1a1e9f734589e6d5c74d0744de8f32 

Branch pushed to git repo; I updated commit sha1. New commits:
edec50e  src/sage/modules/free_module.py, src/sage/modules/fg_pid/fgp_morphism.py: Use ModulesWithBasis instead of FreeModules, which is only an alias defined in sage.categories.all

8230736  FreeModule_submodule_with_basis_pid: Fix up category

comment:9 Changed 3 months ago by
LGTM other than catching Exception
seems quite broad and might hide bugs. Could we make that more refined?
comment:10 Changed 3 months ago by
I generally agree (and specifically it did hide a bug, just fixed in 8230736!) but this is copied directly from Module_free_ambient.__init__
and I don't have special insights to what it should be tightened.
comment:11 Changed 3 months ago by
I would do the standard ones: ValueError
, TypeError
, AttributeError
, and maybe NotImplementedError
. If it is raising something outside of these (such as an assert
) then it almost certainly should be another issue to address.
comment:12 Changed 3 months ago by
Commit:  823073652e1a1e9f734589e6d5c74d0744de8f32 → 2929a9e799ab5a913f753bc4e46b78aaae4180be 

Branch pushed to git repo; I updated commit sha1. New commits:
2929a9e  src/sage/modules/free_module.py: Make 'except Exception' more specific

comment:14 Changed 3 months ago by
+ if category is None: + from sage.categories.modules_with_basis import ModulesWithBasis + category = ModulesWithBasis(R.category()).FiniteDimensional() + try: + if R.is_finite() or len(basis) == 0: + category = category.Enumerated().Finite() + except (ValueError, TypeError, AttributeError, NotImplementedError): + pass + category = category.Subobjects()
I guess I should probably change this to category &= ModulesWithBasis(R.category()).Subobjects()
?
comment:15 Changed 3 months ago by
That would probably be better since it is not promising to be, e.g., a subalgebra.
comment:16 Changed 3 months ago by
Commit:  2929a9e799ab5a913f753bc4e46b78aaae4180be → 62f28a334262b6b6712c0a6d98e07291abcba5e2 

Branch pushed to git repo; I updated commit sha1. New commits:
62f28a3  src/sage/modules/free_module.py: Compute category more carefully

comment:17 Changed 3 months ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Thanks. LGTM.
comment:19 Changed 3 months ago by
Branch:  u/mkoeppe/category_of_submodules_of_sage_modules_free_module_freemodule_ambient_pid → 62f28a334262b6b6712c0a6d98e07291abcba5e2 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
FreeModule_submodule_with_basis_pid.{lift,retract}: New