#29801 closed enhancement (fixed)

FormalPolyhedraModule - (so far finitely generated) free modules

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: geometry Keywords:
Cc: tscrim, 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 mkoeppe)

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 16 months ago by mkoeppe

  • Branch set to u/mkoeppe/formalpolyhedramodule____so_far_finitely_generated__free_modules

comment:2 Changed 16 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to b9d939cda4644785d76063b3d7996866a11c2d34

New commits:

b9d939csage.geometry.polyhedron.modules.formal_polyhedra_module: New

comment:3 Changed 16 months ago by git

  • Commit changed from b9d939cda4644785d76063b3d7996866a11c2d34 to 8721d98249a1cde0c389bae5b83afe92d1163c84

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

8721d98Add __init__.py and imports

comment:4 Changed 16 months ago by mkoeppe

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

comment:5 Changed 16 months ago by tscrim

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 git

  • Commit changed from 8721d98249a1cde0c389bae5b83afe92d1163c84 to fd5bc3494aafafd41283d8e6c90054e62ebba3c4

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

fd5bc34sage.categories.polyhedra_modules: New

comment:7 Changed 16 months ago by mkoeppe

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 tscrim

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 mkoeppe

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 tscrim

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 16 months ago by mkoeppe

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

comment:12 Changed 16 months ago by git

  • Commit changed from fd5bc3494aafafd41283d8e6c90054e62ebba3c4 to b2ba70e6001be24bc5e71842f0b79e009681f9a7

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

a0d791eWIP: add submodule method
b2ba70eUpdate documentation

comment:13 Changed 16 months ago by git

  • Commit changed from b2ba70e6001be24bc5e71842f0b79e009681f9a7 to 5115960d7658f153e6425e7da8e933f1c71e8005

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

5115960Update documentation

comment:14 Changed 16 months ago by git

  • Commit changed from 5115960d7658f153e6425e7da8e933f1c71e8005 to 59889736fe7e3054e942312141236a02ddea325c

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 16 months ago by git

  • Commit changed from 59889736fe7e3054e942312141236a02ddea325c to 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7

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

7ae0c7eAdd to doc

comment:16 Changed 16 months ago by mkoeppe

  • Description modified (diff)
  • Status changed from new to needs_review

comment:17 follow-ups: Changed 16 months ago by 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?

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 16 months ago by mkoeppe

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 16 months ago by mkoeppe

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

comment:20 Changed 16 months ago by git

  • Commit changed from 7ae0c7e5a77cdcb19598db5697bd313e6f84bbb7 to 37aae77b44856eaaf6f416a3229e8456e0b61faf

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 16 months ago by mkoeppe

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 16 months ago by git

  • Commit changed from 37aae77b44856eaaf6f416a3229e8456e0b61faf to bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2

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

bdbd9e4Another formatting fix

comment:23 follow-up: Changed 16 months ago by tscrim

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 git

  • Commit changed from bdbd9e4aa28c2d56e2ec265d56e7aea0b791ced2 to 463dea29704f27a9f23a639b5dedcdf83711d9fb

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 16 months ago by mkoeppe

Done.

comment:26 in reply to: ↑ 23 Changed 16 months ago by mkoeppe

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 16 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you.

comment:28 Changed 16 months ago by mkoeppe

Thanks!

comment:29 Changed 15 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.