Opened 3 years ago
Last modified 2 years ago
#28280 closed task
CombinatorialPolyhedron: replace attributes by methods — at Version 15
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: | d02dee5ee83f3c5a35237da7afa3440f28f47307 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 Polyhedron_base
, or to be more meaningful.
Change History (15)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
- Cc jipilab tscrim vdelecroix added
- Type changed from enhancement to defect
comment:3 Changed 3 years ago by
- Branch set to public/28280
- Commit set to 0aed9fd3024026346291f2dd91e0e1951419bdb1
- Dependencies set to #27987
comment:4 Changed 3 years ago by
- Status changed from new to needs_review
comment:5 Changed 3 years ago by
- Commit changed from 0aed9fd3024026346291f2dd91e0e1951419bdb1 to 2fc4fe01740134a103f301d065c01172b69df3c5
comment:6 Changed 3 years ago by
- Dependencies #27987 deleted
comment:7 follow-up: ↓ 8 Changed 3 years ago by
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
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
- 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
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
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:12 Changed 3 years ago by
- Commit changed from 2fc4fe01740134a103f301d065c01172b69df3c5 to 5f0f7048b82fe5c3d1b8311940e3a7e20967f3bc
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
|
comment:13 Changed 3 years ago by
- Commit changed from 5f0f7048b82fe5c3d1b8311940e3a7e20967f3bc to 7ef7332e37f7e4543c02519f41c605de6ad4aec9
Branch pushed to git repo; I updated commit sha1. New commits:
7ef7332 | meaningful test for graph without edges
|
comment:14 Changed 3 years ago by
- Commit changed from 7ef7332e37f7e4543c02519f41c605de6ad4aec9 to d02dee5ee83f3c5a35237da7afa3440f28f47307
Branch pushed to git repo; I updated commit sha1. New commits:
d02dee5 | typos; length_atom_rep -> n_atom_rep
|
comment:15 Changed 3 years ago by
- Cc gh-LaisRast added
- Description modified (diff)
- Status changed from needs_work to needs_review
Ok, I think naming is consistent with Polyhedron_base
now.
It would be great to have this in sage 8.9, so that we can avoid any deprecation warning.
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. IfCombinatorialPolyhedron
was ever to be a parent ofPolyhedron_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 methodbitrep_facets
and an attribute_bitrep_facets
. This way one can later change this to be lazily evaluated.Any thoughts?