Opened 3 years ago

Last modified 2 years ago

#28280 closed task

CombinatorialPolyhedron: replace attributes by methods — at Version 9

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.

Change History (9)

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

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.

Note: See TracTickets for help on using tickets.