Opened 2 years ago
Closed 2 years ago
#29801 closed enhancement (fixed)
FormalPolyhedraModule  (so far finitely generated) free modules
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  
Cc:  Travis Scrimshaw, ghbraunmath  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  463dea2 (Commits, GitHub, GitLab)  Commit:  463dea29704f27a9f23a639b5dedcdf83711d9fb 
Dependencies:  Stopgaps: 
Description (last modified by )
Add a class FormalPolyhedraModule
for formal modules generated by polyhedra, graded by dimension, and a category PolyhedraModules
.
So far everything is finitely generated. This will be changed in followup tickets.
Change History (29)
comment:1 Changed 2 years ago by
Branch:  → u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules 

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

Commit:  → b9d939cda4644785d76063b3d7996866a11c2d34 
comment:3 Changed 2 years ago by
Commit:  b9d939cda4644785d76063b3d7996866a11c2d34 → 8721d98249a1cde0c389bae5b83afe92d1163c84 

Branch pushed to git repo; I updated commit sha1. New commits:
8721d98  Add __init__.py and imports

comment:4 Changed 2 years ago by
The results of retract
print using a prefix "B". Do I need to override the submodule method?
comment:5 Changed 2 years ago by
This is the default prefix for a submodule, so if you want a different prefix, you will need to override it with something like
def submodule(self, gens, check=True, already_echelonized=False, unitriangular=False, category=None): if not already_echelonized: gens = self.echelon_form(gens, unitriangular) from sage.modules.with_basis.subquotient import SubmoduleWithBasis return SubmoduleWithBasis(gens, ambient=self, prefix='M', unitriangular=unitriangular, category=category)
However, you can set/change this dynamically at any point with:
sage: M_lower.print_options(prefix='V') sage: y V[0]
Although there probably should be an option added to the submodule
to pass along such information to the submodule's constructor...
comment:6 Changed 2 years ago by
Commit:  8721d98249a1cde0c389bae5b83afe92d1163c84 → fd5bc3494aafafd41283d8e6c90054e62ebba3c4 

Branch pushed to git repo; I updated commit sha1. New commits:
fd5bc34  sage.categories.polyhedra_modules: New

comment:7 Changed 2 years ago by
Is the idea of the prefix for the submodule elements to refer to the ambient module, or to distinguish from the ambient module?
comment:8 Changed 2 years ago by
I am not quite sure I understand your question. All CFMs have a prefix parameter that can be set by the user to help them distinguish between the different *modules they are working with. This is used extensively in Sage (e.g., symmetric functions).
comment:9 Changed 2 years ago by
I guess I need to ask a more basic question first. In the example:
sage: M.basis() Finite family {conv([0], [1]): [conv([0], [1])], {[1]}: [{[1]}], conv([1], [2]): [conv([1], [2])]} sage: M_lower.basis() Finite family {0: B[0]}
How come these two bases (of the ambient module and of the submodule) are indexed in very different ways?
comment:10 Changed 2 years ago by
A submodule of dimension d just uses the indexing set 0,...,d1 because any generic submodule would not necessarily have any particular way to relate its basis with that of the ambient module. Moreover, it needs some indexing set for the basis, so it just chooses the simplest and most generic one. One possible way would be to use the leading support of each of the echeleonized basis elements, but that still feels fairly artificial/arbitrary to me.
comment:11 Changed 2 years ago by
OK, thanks a lot for the explanation. I think I will revisit this cosmetic issue later.
comment:12 Changed 2 years ago by
Commit:  fd5bc3494aafafd41283d8e6c90054e62ebba3c4 → b2ba70e6001be24bc5e71842f0b79e009681f9a7 

comment:13 Changed 2 years ago by
Commit:  b2ba70e6001be24bc5e71842f0b79e009681f9a7 → 5115960d7658f153e6425e7da8e933f1c71e8005 

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

comment:14 Changed 2 years ago by
Commit:  5115960d7658f153e6425e7da8e933f1c71e8005 → 59889736fe7e3054e942312141236a02ddea325c 

Branch pushed to git repo; I updated commit sha1. New commits:
5988973  Update doctests to set prefixes for distinguishing the ambient, sub, quotient

comment:15 Changed 2 years ago by
Commit:  59889736fe7e3054e942312141236a02ddea325c → 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7 

Branch pushed to git repo; I updated commit sha1. New commits:
7ae0c7e  Add to doc

comment:16 Changed 2 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
comment:17 followups: 18 21 Changed 2 years ago by
At this point, I don't see the need for having a category PolyhedraModules
. Perhaps this is something you will expand on later, but currently you can just put the degree_on_basis
in the FormalPolyhedraModule
(which would belong to the category of GradedModulesWithBasis
). So can you explain a bit why we should have this category?
If you do keep the category, super_categories
needs a doctest.
Your INPUT:
block should follow this format:
INPUT:  ``foo``  some information  ``bar``  other information that is really long
Note the alignment (you have an additional 1 space indentation) and the double 
for the nonbullet point.
FormalPolyhedraModule.__init__
needs a doctest; here we usually do a TestSuite(M).run()
as the test.
comment:18 Changed 2 years ago by
Replying to tscrim:
At this point, I don't see the need for having a category
PolyhedraModules
. Perhaps this is something you will expand on later, but currently you can just put thedegree_on_basis
in theFormalPolyhedraModule
(which would belong to the category ofGradedModulesWithBasis
). So can you explain a bit why we should have this category?
It is indeed for future use, when I introduce the space generated by indicator functions of polyhedra, which is the quotient by all inclusionexclusion relations. That space is no longer graded, but only filtered by dimension.
comment:19 Changed 2 years ago by
But I'll follow your advice and make it just a method of the class for now.
comment:20 Changed 2 years ago by
Commit:  7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7 → 37aae77b44856eaaf6f416a3229e8456e0b61faf 

Branch pushed to git repo; I updated commit sha1. New commits:
37aae77  Remove category, merge method degree_on_basis into the class

comment:21 Changed 2 years ago by
Replying to tscrim:
Your
INPUT:
block should follow this format... Note the alignment (you have an additional 1 space indentation) and the double
for the nonbullet point.
Thanks, fixed.
I really wish we had a linter for the docstrings that we could run locally instead of wasting reviewers' time with these formatting details and/or waiting for the patchbot...
comment:22 Changed 2 years ago by
Commit:  37aae77b44856eaaf6f416a3229e8456e0b61faf → bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2 

Branch pushed to git repo; I updated commit sha1. New commits:
bdbd9e4  Another formatting fix

comment:23 followup: 26 Changed 2 years ago by
Sorry, that wasn't meant to be a suggestion to remove it; it was purely a question as I don't know what your long term structural plans are. Although I do think purely local to this ticket it is better.
Your __init__
still needs a doctest, something like
sage: from sage.geometry.polyhedron.modules.formal_polyhedra_module import FormalPolyhedraModule sage: def closed_interval(a,b): return Polyhedron(vertices=[[a], [b]]) sage: I01 = closed_interval(0, 1); I01.rename("conv([0], [1])") sage: I11 = closed_interval(1, 1); I11.rename("{[1]}") sage: I12 = closed_interval(1, 2); I12.rename("conv([1], [2])") sage: I02 = closed_interval(0, 2); I02.rename("conv([0], [2])") sage: M = FormalPolyhedraModule(QQ, 1, basis=[I01, I11, I12, I02]) sage: TestSuite(M).run()
To help avoid some confusion in degree_on_basis
, I think it would be a bit better to say this would only be a filtration,
since this module does not (yet) have the linear relations implemented and it is in the category of graded modules.
Indeed, such a linter would be nice.
comment:24 Changed 2 years ago by
Commit:  bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2 → 463dea29704f27a9f23a639b5dedcdf83711d9fb 

comment:26 Changed 2 years ago by
comment:27 Changed 2 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Thank you.
comment:29 Changed 2 years ago by
Branch:  u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules → 463dea29704f27a9f23a639b5dedcdf83711d9fb 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
sage.geometry.polyhedron.modules.formal_polyhedra_module: New