Opened 16 months ago
Closed 15 months ago
#29801 closed enhancement (fixed)
FormalPolyhedraModule  (so far finitely generated) free modules
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  
Cc:  tscrim, 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 16 months ago by
 Branch set to u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules
comment:2 Changed 16 months ago by
 Commit set to b9d939cda4644785d76063b3d7996866a11c2d34
comment:3 Changed 16 months ago by
 Commit changed from b9d939cda4644785d76063b3d7996866a11c2d34 to 8721d98249a1cde0c389bae5b83afe92d1163c84
Branch pushed to git repo; I updated commit sha1. New commits:
8721d98  Add __init__.py and imports

comment:4 Changed 16 months ago by
The results of retract
print using a prefix "B". Do I need to override the submodule method?
comment:5 Changed 16 months 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 16 months ago by
 Commit changed from 8721d98249a1cde0c389bae5b83afe92d1163c84 to fd5bc3494aafafd41283d8e6c90054e62ebba3c4
Branch pushed to git repo; I updated commit sha1. New commits:
fd5bc34  sage.categories.polyhedra_modules: New

comment:7 Changed 16 months 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 16 months 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 16 months 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 16 months 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 16 months ago by
OK, thanks a lot for the explanation. I think I will revisit this cosmetic issue later.
comment:12 Changed 16 months ago by
 Commit changed from fd5bc3494aafafd41283d8e6c90054e62ebba3c4 to b2ba70e6001be24bc5e71842f0b79e009681f9a7
comment:13 Changed 16 months ago by
 Commit changed from b2ba70e6001be24bc5e71842f0b79e009681f9a7 to 5115960d7658f153e6425e7da8e933f1c71e8005
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5115960  Update documentation

comment:14 Changed 16 months ago by
 Commit changed from 5115960d7658f153e6425e7da8e933f1c71e8005 to 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 16 months ago by
 Commit changed from 59889736fe7e3054e942312141236a02ddea325c to 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7
Branch pushed to git repo; I updated commit sha1. New commits:
7ae0c7e  Add to doc

comment:16 Changed 16 months ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:17 followups: ↓ 18 ↓ 21 Changed 16 months 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 in reply to: ↑ 17 Changed 16 months 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 16 months ago by
But I'll follow your advice and make it just a method of the class for now.
comment:20 Changed 16 months ago by
 Commit changed from 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7 to 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 in reply to: ↑ 17 Changed 16 months 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 16 months ago by
 Commit changed from 37aae77b44856eaaf6f416a3229e8456e0b61faf to bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2
Branch pushed to git repo; I updated commit sha1. New commits:
bdbd9e4  Another formatting fix

comment:23 followup: ↓ 26 Changed 16 months 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 16 months ago by
 Commit changed from bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2 to 463dea29704f27a9f23a639b5dedcdf83711d9fb
comment:25 Changed 16 months ago by
Done.
comment:26 in reply to: ↑ 23 Changed 16 months ago by
comment:27 Changed 16 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you.
comment:28 Changed 16 months ago by
Thanks!
comment:29 Changed 15 months ago by
 Branch changed from u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules to 463dea29704f27a9f23a639b5dedcdf83711d9fb
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
sage.geometry.polyhedron.modules.formal_polyhedra_module: New