Opened 3 years ago
Closed 20 months ago
#28280 closed task (worksforme)
Task: CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  geometry  Keywords:  
Cc:  jipilab, tscrim, vdelecroix, ghLaisRast, chapoton  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket gathers tickets related to the `CombinatorialPolyhedron?' class.
The goal is to make this class consistent with the base class Polyhedron_base
.
 #28603:
edge_graph
>vertex_graph
.  #28604:
ridge_graph
>facet_graph
.  #28605: Replace attributes in
CombinatorialPolyhedron
by methods, such that they can potentially be lazily evaluated. This is motivated by #10777.  #28606:
unbounded(self)
>is_bounded(self)
.  #28607: Make
f_vector
a vector.  #28613: Replace
V
andH
by more meaningful attributes/methods.  #28614:
length_*
>n_*
.  #28616: Replace
Vrepr()
andHrepr
by more consistent methods inCombinatorialFace
.  #28608:
repr
>rep
when abbreviating.  #28757: Remove empty folder
 #29110: Make
dim
and alias fordimension
in combinatorial polyhedron  #29242: CombinatorialPolyhedron?:
bit_repr_
>bit_rep_
Some of the tickets also take care of minor bug fixes or typos.
Change History (40)
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 2 years ago by
 Commit changed from 0aed9fd3024026346291f2dd91e0e1951419bdb1 to 2fc4fe01740134a103f301d065c01172b69df3c5
comment:6 Changed 2 years ago by
 Dependencies #27987 deleted
comment:7 followup: ↓ 8 Changed 2 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 2 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 2 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 2 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 2 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
comment:12 Changed 2 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 2 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 2 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 2 years ago by
 Cc ghLaisRast 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.
comment:16 followup: ↓ 17 Changed 2 years ago by
 Status changed from needs_review to needs_work
Well... 8.9.rc0 is already out... so time is running out... :(
On which version of Sage is this based? Could you rebase on 8.9.rc0 so that we have a clear view of what is done in this ticket. Or is it actually already only stuff from this ticket?
 I would vote for
Vrep>Vrepresentation
same forH
. This is what I meant originally with be consistent withPolyhedron
, I should have specified it, my bad.
 You should also mention in the description of this ticket that you remove an empty folder. This is done in one of the commits. (This is a good habit to take for such small changes, otherwise, the penalty is: "please open a ticket just for that"...).
comment:17 in reply to: ↑ 16 ; followup: ↓ 18 Changed 2 years ago by
Replying to jipilab:
Well... 8.9.rc0 is already out... so time is running out... :(
On which version of Sage is this based? Could you rebase on 8.9.rc0 so that we have a clear view of what is done in this ticket. Or is it actually already only stuff from this ticket?
This ticket is huge, but almost all changes are trivial. Sorry, but that's what happens, when you change some names.
 I would vote for
Vrep>Vrepresentation
same forH
. This is what I meant originally with be consistent withPolyhedron
, I should have specified it, my bad.
In Polyhedron
there is Vrepresentation
and Vrepgenerator
. So in a way, both things are used.
Now for CombinatorialPolyhedron
there is only Vrepresentation
in the namespace. _Vrep
is an internal cython variable, while Vrep
is the cython method that accesses _Vrep
. I tried to use cpdef
for both of them, but that didn't work (closures inside cpdef functions not yet supported
, seems to be connected with smallInteger
). So as long as it is that way, I would propse leaving the python method named Vrepresentation
and the cython method Vrepr
. But I'm open for different suggestions.
Now with CombinatorialFace
things are a bit different. At the moment there is Vrep
, Hrep
, n_Vrepresentation
and n_Hrepresentation
(not consistent, I just noticed). Vrep
allows giving both the indices and the names of the Vrepresentation. So in a way it combines ambient_Vrepresentation
and ambient_V_indices
of PolyhedronFace
. What do you suggest?
One option would be the following for CombinatorialFace
:
Vrep
>ambient_Vrep
 make
ambient_Vrepresentation
an alias forambient_Vrep(names=True)
 make
ambient_V_indices
an alias forambient_V_indices(names=False)
n_Vrepresentation
>n_ambient_Vreprentation
 same for
Hrep
Unfortunatly, those changes are going to make the ticket even larger.
What do you think? If I have a clear idea what to do, I think I can make the remaining changes tonight.
 You should also mention in the description of this ticket that you remove an empty folder. This is done in one of the commits. (This is a good habit to take for such small changes, otherwise, the penalty is: "please open a ticket just for that"...).
comment:18 in reply to: ↑ 17 ; followup: ↓ 19 Changed 2 years ago by
Replying to ghkliem:
Replying to jipilab:
Well... 8.9.rc0 is already out... so time is running out... :(
On which version of Sage is this based? Could you rebase on 8.9.rc0 so that we have a clear view of what is done in this ticket. Or is it actually already only stuff from this ticket?
This ticket is huge, but almost all changes are trivial. Sorry, but that's what happens, when you change some names.
I was just making sure. Let's try to make it happen.
 I would vote for
Vrep>Vrepresentation
same forH
. This is what I meant originally with be consistent withPolyhedron
, I should have specified it, my bad.In
Polyhedron
there isVrepresentation
andVrepgenerator
. So in a way, both things are used.
V_rep
is shortened because _generator
appears after. So I would vote for completing the name to Vrepresentation
.
Now for
CombinatorialPolyhedron
there is onlyVrepresentation
in the namespace._Vrep
is an internal cython variable, whileVrep
is the cython method that accesses_Vrep
. I tried to usecpdef
for both of them, but that didn't work (closures inside cpdef functions not yet supported
, seems to be connected withsmallInteger
). So as long as it is that way, I would propse leaving the python method namedVrepresentation
and the cython methodVrepr
. But I'm open for different suggestions.
Got it! Looks good.
Now with
CombinatorialFace
things are a bit different. At the moment there isVrep
,Hrep
,n_Vrepresentation
andn_Hrepresentation
(not consistent, I just noticed).Vrep
allows giving both the indices and the names of the Vrepresentation. So in a way it combinesambient_Vrepresentation
andambient_V_indices
ofPolyhedronFace
. What do you suggest?
Ok. I see. I would do Vrep
to Vrepresentation
. Then, if CombinatorialFace
has a different behavior than the PolyhedronFace
I believe it is fine.
One option would be the following for
CombinatorialFace
:
Vrep
>ambient_Vrep
 make
ambient_Vrepresentation
an alias forambient_Vrep(names=True)
 make
ambient_V_indices
an alias forambient_V_indices(names=False)
n_Vrepresentation
>n_ambient_Vreprentation
 same for
Hrep
It seems like making the current Vrep
private and have ambient_Vrepresentation
(with the whole word) call this private function Vrep(names=True)
and likewise for ambient_V_indices
be more reasonable.
Why have 3 functions when we can have 2? Especially, the 2 functions behave as they do in PolyhedronFace
... so I do not see the need to provide Vrep
in the namespace.
 You should also mention in the description of this ticket that you remove an empty folder. This is done in one of the commits. (This is a good habit to take for such small changes, otherwise, the penalty is: "please open a ticket just for that"...).
Please apply this change to the description.
comment:19 in reply to: ↑ 18 Changed 2 years ago by
Replying to jipilab:
Replying to ghkliem:
Replying to jipilab:
Well... 8.9.rc0 is already out... so time is running out... :(
On which version of Sage is this based? Could you rebase on 8.9.rc0 so that we have a clear view of what is done in this ticket. Or is it actually already only stuff from this ticket?
This ticket is huge, but almost all changes are trivial. Sorry, but that's what happens, when you change some names.
I was just making sure. Let's try to make it happen.
 I would vote for
Vrep>Vrepresentation
same forH
. This is what I meant originally with be consistent withPolyhedron
, I should have specified it, my bad.In
Polyhedron
there isVrepresentation
andVrepgenerator
. So in a way, both things are used.
V_rep
is shortened because_generator
appears after. So I would vote for completing the name toVrepresentation
.Now for
CombinatorialPolyhedron
there is onlyVrepresentation
in the namespace._Vrep
is an internal cython variable, whileVrep
is the cython method that accesses_Vrep
. I tried to usecpdef
for both of them, but that didn't work (closures inside cpdef functions not yet supported
, seems to be connected withsmallInteger
). So as long as it is that way, I would propse leaving the python method namedVrepresentation
and the cython methodVrepr
. But I'm open for different suggestions.Got it! Looks good.
Now with
CombinatorialFace
things are a bit different. At the moment there isVrep
,Hrep
,n_Vrepresentation
andn_Hrepresentation
(not consistent, I just noticed).Vrep
allows giving both the indices and the names of the Vrepresentation. So in a way it combinesambient_Vrepresentation
andambient_V_indices
ofPolyhedronFace
. What do you suggest?Ok. I see. I would do
Vrep
toVrepresentation
. Then, ifCombinatorialFace
has a different behavior than thePolyhedronFace
I believe it is fine.One option would be the following for
CombinatorialFace
:
Vrep
>ambient_Vrep
 make
ambient_Vrepresentation
an alias forambient_Vrep(names=True)
 make
ambient_V_indices
an alias forambient_V_indices(names=False)
n_Vrepresentation
>n_ambient_Vreprentation
 same for
Hrep
It seems like making the current
Vrep
private and haveambient_Vrepresentation
(with the whole word) call this private functionVrep(names=True)
and likewise forambient_V_indices
be more reasonable.Why have 3 functions when we can have 2? Especially, the 2 functions behave as they do in
PolyhedronFace
... so I do not see the need to provideVrep
in the namespace.
 You should also mention in the description of this ticket that you remove an empty folder. This is done in one of the commits. (This is a good habit to take for such small changes, otherwise, the penalty is: "please open a ticket just for that"...).
Please apply this change to the description.
I will. It's just one of those things that where perfectly clear to me, so I do didn't comment.
One more thing: Lots of changes are due to the changes of abbreciations Vrepr
> Vrep
etc. I noticed you seem to at least use both. I could undo this change and this might make the ticket shorter.
comment:20 Changed 2 years ago by
 Commit changed from d02dee5ee83f3c5a35237da7afa3440f28f47307 to 9d506b820f1c245b5f6ca769506d2ae58a47095e
comment:21 Changed 2 years ago by
 Description modified (diff)
 Summary changed from CombinatorialPolyhedron: replace attributes by methods to CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron
comment:22 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 2 years ago by
 Milestone changed from sage8.9 to sage9.0
 Status changed from needs_review to needs_work
I guess, we need deprecation warnings now.
comment:24 Changed 2 years ago by
 Branch public/28280 deleted
 Commit 9d506b820f1c245b5f6ca769506d2ae58a47095e deleted
 Dependencies set to #28605
 Description modified (diff)
 Type changed from enhancement to task
comment:25 Changed 2 years ago by
 Dependencies changed from #28605 to #28603, #28604, #28605, #28606, #28607, #28608, #28613, #28614, #28615, #28616
comment:26 Changed 2 years ago by
 Status changed from needs_work to needs_review
I'm going to put this on needs review as long as tickets on that list need to be reviewed.
So on #22420 (meta ticket polyhedra) there needs to be only this ticket to collect all the others.
comment:27 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:28 Changed 2 years ago by
 Description modified (diff)
 Summary changed from CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron to Task: CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron
comment:29 Changed 2 years ago by
 Dependencies #28603, #28604, #28605, #28606, #28607, #28608, #28613, #28614, #28615, #28616 deleted
 Description modified (diff)
comment:30 Changed 2 years ago by
 Description modified (diff)
comment:31 Changed 2 years ago by
 Description modified (diff)
comment:32 Changed 2 years ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:33 Changed 2 years ago by
 Description modified (diff)
comment:34 Changed 2 years ago by
 Description modified (diff)
comment:35 Changed 22 months ago by
 Milestone changed from sage9.1 to sage9.2
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.
comment:36 Changed 21 months ago by
 Milestone changed from sage9.2 to sageduplicate/invalid/wontfix
 Status changed from needs_work to needs_review
This task is taken care of.
comment:37 Changed 21 months ago by
 Cc chapoton added
Please, close this ticket as it's objectives have been fulfilled! Thanks!
comment:38 Changed 21 months ago by
 Reviewers set to JeanPhilippe Labbé
comment:39 Changed 21 months ago by
 Status changed from needs_review to positive_review
comment:40 Changed 20 months ago by
 Resolution set to worksforme
 Status changed from positive_review to closed
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?
Any thoughts?