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: sage-9.2
Component: geometry Keywords:
Cc: Travis Scrimshaw, gh-braunmath Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 463dea2 (Commits, GitHub, GitLab) Commit: 463dea29704f27a9f23a639b5dedcdf83711d9fb
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

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 follow-up tickets.

Change History (29)

comment:1 Changed 2 years ago by Matthias Köppe

Branch: u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules

comment:2 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe
Commit: b9d939cda4644785d76063b3d7996866a11c2d34

New commits:

b9d939csage.geometry.polyhedron.modules.formal_polyhedra_module: New

comment:3 Changed 2 years ago by git

Commit: b9d939cda4644785d76063b3d7996866a11c2d348721d98249a1cde0c389bae5b83afe92d1163c84

Branch pushed to git repo; I updated commit sha1. New commits:

8721d98Add __init__.py and imports

comment:4 Changed 2 years ago by Matthias Köppe

The results of retract print using a prefix "B". Do I need to override the submodule method?

comment:5 Changed 2 years ago by Travis Scrimshaw

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 git

Commit: 8721d98249a1cde0c389bae5b83afe92d1163c84fd5bc3494aafafd41283d8e6c90054e62ebba3c4

Branch pushed to git repo; I updated commit sha1. New commits:

fd5bc34sage.categories.polyhedra_modules: New

comment:7 Changed 2 years ago by Matthias Köppe

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 Travis Scrimshaw

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 Matthias Köppe

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 Travis Scrimshaw

A submodule of dimension d just uses the indexing set 0,...,d-1 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 Matthias Köppe

OK, thanks a lot for the explanation. I think I will revisit this cosmetic issue later.

comment:12 Changed 2 years ago by git

Commit: fd5bc3494aafafd41283d8e6c90054e62ebba3c4b2ba70e6001be24bc5e71842f0b79e009681f9a7

Branch pushed to git repo; I updated commit sha1. New commits:

a0d791eWIP: add submodule method
b2ba70eUpdate documentation

comment:13 Changed 2 years ago by git

Commit: b2ba70e6001be24bc5e71842f0b79e009681f9a75115960d7658f153e6425e7da8e933f1c71e8005

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

5115960Update documentation

comment:14 Changed 2 years ago by git

Commit: 5115960d7658f153e6425e7da8e933f1c71e800559889736fe7e3054e942312141236a02ddea325c

Branch pushed to git repo; I updated commit sha1. New commits:

5988973Update doctests to set prefixes for distinguishing the ambient, sub, quotient

comment:15 Changed 2 years ago by git

Commit: 59889736fe7e3054e942312141236a02ddea325c7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7

Branch pushed to git repo; I updated commit sha1. New commits:

7ae0c7eAdd to doc

comment:16 Changed 2 years ago by Matthias Köppe

Description: modified (diff)
Status: newneeds_review

comment:17 Changed 2 years ago by Travis Scrimshaw

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 non-bullet point.

FormalPolyhedraModule.__init__ needs a doctest; here we usually do a TestSuite(M).run() as the test.

comment:18 in reply to:  17 Changed 2 years ago by Matthias Köppe

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 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?

It is indeed for future use, when I introduce the space generated by indicator functions of polyhedra, which is the quotient by all inclusion-exclusion relations. That space is no longer graded, but only filtered by dimension.

comment:19 Changed 2 years ago by Matthias Köppe

But I'll follow your advice and make it just a method of the class for now.

comment:20 Changed 2 years ago by git

Commit: 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb737aae77b44856eaaf6f416a3229e8456e0b61faf

Branch pushed to git repo; I updated commit sha1. New commits:

37aae77Remove category, merge method degree_on_basis into the class

comment:21 in reply to:  17 Changed 2 years ago by Matthias Köppe

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 non-bullet 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 git

Commit: 37aae77b44856eaaf6f416a3229e8456e0b61fafbdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2

Branch pushed to git repo; I updated commit sha1. New commits:

bdbd9e4Another formatting fix

comment:23 Changed 2 years ago by Travis Scrimshaw

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 git

Commit: bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2463dea29704f27a9f23a639b5dedcdf83711d9fb

Branch pushed to git repo; I updated commit sha1. New commits:

aecc78aUpdate doc and link into reference manual
463dea2FormalPolyhedraModule.__init__: Add test

comment:25 Changed 2 years ago by Matthias Köppe

Done.

comment:26 in reply to:  23 Changed 2 years ago by Matthias Köppe

Replying to tscrim:

Indeed, such a linter would be nice.

Meta-ticket #28936 (Adopt mainstream Python testing/linting infrastructure, describe in Developer's Guide) has several wishlist items in this direction.

comment:27 Changed 2 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Thank you.

comment:28 Changed 2 years ago by Matthias Köppe

Thanks!

comment:29 Changed 2 years ago by Volker Braun

Branch: u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules463dea29704f27a9f23a639b5dedcdf83711d9fb
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.