Opened 3 years ago

Last modified 2 years ago

#28280 closed task

CombinatorialPolyhedron: replace attributes by methods — at Version 11

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: geometry Keywords:
Cc: jipilab, tscrim, vdelecroix, gh-LaisRast, chapoton Merged in:
Authors: Jonathan Kliem Reviewers:
Report Upstream: N/A Work issues:
Branch: public/28280 (Commits, GitHub, GitLab) Commit: 2fc4fe01740134a103f301d065c01172b69df3c5
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

Replace attributes in CombinatorialPolyhedron by methods, such that they can potentially be lazily evaluated.

This is motivated by #10777.

While at it, we will change some names of methods of CombinatorialPolyhedron to be consistent with PolyhedronBase, or to be more meaningful.

Change History (11)

comment:1 Changed 3 years ago by gh-kliem

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?

Last edited 3 years ago by gh-kliem (previous) (diff)

comment:2 Changed 3 years ago by gh-kliem

  • Cc jipilab tscrim vdelecroix added
  • Type changed from enhancement to defect

comment:3 Changed 3 years ago by gh-kliem

  • Authors set to Jonathan Kliem
  • Branch set to public/28280
  • Commit set to 0aed9fd3024026346291f2dd91e0e1951419bdb1
  • Dependencies set to #27987

New commits:

d6d663acombinatorial polyhedron uses far face as a bug fix
14d17a8added doctest reporting the bug fix
806a217correct link to trac ticket, if unbounded is False -> if not unbounded
9194627replace attributes by methods
0aed9fdremoved empty folder being created in source

comment:4 Changed 3 years ago by gh-kliem

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by git

  • Commit changed from 0aed9fd3024026346291f2dd91e0e1951419bdb1 to 2fc4fe01740134a103f301d065c01172b69df3c5

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

d597ed3replace attributes by methods
2fc4fe0removed empty folder being created in source

comment:6 Changed 3 years ago by gh-kliem

  • Dependencies #27987 deleted

comment:7 follow-up: Changed 3 years ago by tscrim

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

comment:8 in reply to: ↑ 7 Changed 3 years ago by gh-kliem

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

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.

comment:9 Changed 3 years ago by gh-kliem

  • Description modified (diff)
  • Type changed from defect to enhancement

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.

comment:10 Changed 3 years ago by jipilab

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...).

comment:11 Changed 3 years ago by gh-kliem

  • Description modified (diff)
  • Status changed from needs_review to needs_work
Note: See TracTickets for help on using tickets.