Task: CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron

[kliem]

This ticket gathers tickets related to the `CombinatorialPolyhedron' class.

The goal is to make this class consistent with the base class Polyhedron_base.

• CombinatorialPolyhedron: edge_graph -> vertex_graph #28603: edge_graph -> vertex_graph.
• CombinatorialPolyhedron: ridge_graph -> facet_graph #28604: ridge_graph -> facet_graph.
• CombinatorialPolyhedron: replace attributes by methods #28605: Replace attributes in CombinatorialPolyhedron by methods, such that they can potentially be lazily evaluated.
  This is motivated by Make CombinatorialPolyhedron a base class for polyhedra/lattice polytopes/cones #10777.
• CombinatorialPolyhedron: unbounded -> is_bounded #28606: unbounded(self) -> is_bounded(self).
• CombinatorialPolyhedron: make f_vector a vector #28607: Make f_vector a vector.
• CombinatorialPolyhedron: attributes, H -> facet_names; V -> Vrep #28613: Replace V and H by more meaningful attributes/methods.
• CombinatorialPolyhedron: length_* to n_* #28614: length_* -> n_*.
• CombinatorialFace: replace Vrepr() and Hrepr() by more consistent names #28616: Replace Vrepr() and Hrepr by more consistent methods in CombinatorialFace.
• CombinatorialPolyhedron: abbrevations Hrepr -> Hrep etc. #28608: repr -> rep when abbreviating.
• CombinatorialPolyhedron: Remove empty folder #28757: Remove empty folder
• Make dim and alias for dimension in combinatorial polyhedron #29110: Make dim and alias for dimension in combinatorial polyhedron
• CombinatorialPolyhedron: bit_repr_ -> bit_rep_ #29242: CombinatorialPolyhedron: bit_repr_ -> bit_rep_

Some of the tickets also take care of minor bug fixes or typos.

CC: @jplab @tscrim @videlec @LaisRast @fchapoton

Component: geometry

Author: Jonathan Kliem

Reviewer: Jean-Philippe Labbé

Issue created by migration from https://trac.sagemath.org/ticket/28280

[kliem]

comment:1

Some attributes like bitrep_vertices might have been an unfortunate choice:

As they are accessed by other classes, I'm forced to compute e.g. bitrep_vertices on initialization. If CombinatorialPolyhedron was ever to be a parent of Polyhedron_base (see #10777), this would force a polyhedron to compute the incidence matrix right away.

From my understanding, the decorator @lazy_attribute is not available for extension types directly. I'm thinking of changing those attributes to methods, such that there is a method bitrep_facets and an attribute _bitrep_facets. This way one can later change this to be lazily evaluated.

Any thoughts?

[kliem]

Branch: public/28280

[kliem]

Dependencies: #27987

[kliem]

Author: Jonathan Kliem

[kliem]

Commit: 0aed9fd

[kliem]

New commits:

d6d663a	combinatorial polyhedron uses far face as a bug fix
14d17a8	added doctest reporting the bug fix
806a217	correct link to trac ticket, if unbounded is False -> if not unbounded
9194627	replace attributes by methods
0aed9fd	removed empty folder being created in source

[sagetrac-git]

Changed commit from 0aed9fd to 2fc4fe0

[sagetrac-git]

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

d597ed3	replace attributes by methods
2fc4fe0	removed empty folder being created in source

[kliem]

Changed dependencies from #27987 to none

[tscrim]

comment:7

If a @lazy_attribute doesn't work, then just make it a @cached_method (with no arguments).

[kliem]

comment:8

Replying to @tscrim:

If a @lazy_attribute doesn't work, then just make it a @cached_method (with no arguments).

Does either one work with cdef?

Here, I want to give a small example of what I want to do:

sage: cython('''
....: cdef class BaseClass:
....:     cdef public int number
....: 
....:     def __init__(self):
....:         self.number = 2
....: ''')
sage: class NewClass(BaseClass):
....:     def __init__(self):
....:         pass
....:     
sage: example = NewClass()
sage: example.number
0

I'm trying to somehow make number be a lazy attribute. The only solution I came up with, is to replace it by a method. Is there another way?

The background is that CombinatorialPolyhedron does require Polyhedron to compute the incidence matrix for correct initialization. If CombinatorialPolyhedron is to be a base class of Polyhedron_base at some point (#10777), it needs to be lazily initialized.

[kliem]

comment:9

Is this the right approach, am I missing something? Is it acceptable to do those name changes? (CombinatorialPolyhedron still was not part of a stable release)

If so, I think the review is rather trivial and I can try to find someone else to do it.

[jplab]

comment:10

Some comments:

Since these methods will then occur in the namespace, perhaps it would make sense to make it consistent with the names of the methods for Polyhedron. For example, unbounded -> is_(un)bounded. Probably it would make more sense to store is_bounded rather than unbounded....

Similar for V and H, having a method with only one letter is not so revealing...

I guess it is important to be careful with the naming at this precise moment as after, changing the name of methods will force the usage of deprecation warnings and that's annoying. (I believe that some methods should be changed to be consistent with Polyhedron...).

[sagetrac-git]

Changed commit from 2fc4fe0 to 5f0f704

[sagetrac-git]

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

037b3ed	ridge_graph -> facet_graph; some bug fixing
07e4ea7	f_vector is a vector now
c18a757	abbreavations: Hrepr -> Hrep, Vrepr -> Vrep etc.
4d165f5	unbounded -> bounded, bounded(self) -> is_bounded(self)
bbd5c74	length_Hrep -> n_Hrepresentation; likewise for Vrep
21b9e03	removed attribute Vinv, as its not being used
c844ec5	made specification to n_Hrepresentation in combinatorial_face
7403c4b	Hrep -> facet_names
b8cd255	V -> Vrep
5f0f704	added docstrings to the new methods

[sagetrac-git]

Changed commit from 5f0f704 to 7ef7332

[sagetrac-git]

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

7ef7332

meaningful test for graph without edges

[kliem]

Changed dependencies from #28603, #28604, #28605, #28606, #28607, #28608, #28613, #28614, #28615, #28616 to none

[embray]

comment:32

Ticket retargeted after milestone closed

[mkoeppe]

comment:35

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

[kliem]

comment:36

This task is taken care of.

[jplab]

comment:37

Please, close this ticket as it's objectives have been fulfilled! Thanks!

[jplab]

Reviewer: Jean-Philippe Labbé