Opened 3 years ago
Last modified 2 years ago
#28280 closed task
CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron — at Version 21
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: | 9d506b820f1c245b5f6ca769506d2ae58a47095e |
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 for consistency:
We make CombinatorialPolyhedron
more consistent with Polyhedron_base
, most importantly:
edge_graph
->vertex_graph
,ridge_graph
->facet_graph
,unbounded(self)
->is_bounded(self)
,length_Hrep
->n_Hrepresentation
,length_Hrep
->n_Hrepresentation
.
For CombinatorialFace
we do the following changes:
face.Vrep(True)
->face.ambient_Vrepresentation()
,face.Vrep(False)
->face.ambient_V_indices()
,n_Vrepresentation
->n_ambient_Vrepresentation
,- the above applied to
Hrep
.
We also delete .cc
in the module name of bit_vector_operations
. The .cc
lead to an empty folder in the source directory. Without this folder does not reappear.
In addition we do some minor bug fixing and some improvement in the documentation.
Change History (21)
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.
comment:16 follow-up: ↓ 17 Changed 3 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 ; follow-up: ↓ 18 Changed 3 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 Vrep-generator
. 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 ; follow-up: ↓ 19 Changed 3 years ago by
Replying to gh-kliem:
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
andVrep-generator
. 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 3 years ago by
Replying to jipilab:
Replying to gh-kliem:
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
andVrep-generator
. 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 3 years ago by
- Commit changed from d02dee5ee83f3c5a35237da7afa3440f28f47307 to 9d506b820f1c245b5f6ca769506d2ae58a47095e
comment:21 Changed 3 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
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?