Opened 2 years ago

Closed 15 months ago

#28280 closed task (worksforme)

Task: CombinatorialPolyhedron: replace attributes by methods, make names more consistent with Polyhedron

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: Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

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 and H by more meaningful attributes/methods.
  • #28614: length_* -> n_*.
  • #28616: Replace Vrepr() and Hrepr by more consistent methods in CombinatorialFace.
  • #28608: repr -> rep when abbreviating.
  • #28757: Remove empty folder
  • #29110: Make dim and alias for dimension 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 2 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 2 years ago by gh-kliem (previous) (diff)

comment:2 Changed 2 years ago by gh-kliem

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

comment:3 Changed 2 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 2 years ago by gh-kliem

  • Status changed from new to needs_review

comment:5 Changed 2 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 2 years ago by gh-kliem

  • Dependencies #27987 deleted

comment:7 follow-up: Changed 2 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 2 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 2 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 2 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 2 years ago by gh-kliem

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:12 Changed 2 years ago by git

  • Commit changed from 2fc4fe01740134a103f301d065c01172b69df3c5 to 5f0f7048b82fe5c3d1b8311940e3a7e20967f3bc

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

037b3edridge_graph -> facet_graph; some bug fixing
07e4ea7f_vector is a vector now
c18a757abbreavations: Hrepr -> Hrep, Vrepr -> Vrep etc.
4d165f5unbounded -> bounded, bounded(self) -> is_bounded(self)
bbd5c74length_Hrep -> n_Hrepresentation; likewise for Vrep
21b9e03removed attribute Vinv, as its not being used
c844ec5made specification to n_Hrepresentation in combinatorial_face
7403c4bHrep -> facet_names
b8cd255V -> Vrep
5f0f704added docstrings to the new methods

comment:13 Changed 2 years ago by git

  • Commit changed from 5f0f7048b82fe5c3d1b8311940e3a7e20967f3bc to 7ef7332e37f7e4543c02519f41c605de6ad4aec9

Branch pushed to git repo; I updated commit sha1. New commits:

7ef7332meaningful test for graph without edges

comment:14 Changed 2 years ago by git

  • Commit changed from 7ef7332e37f7e4543c02519f41c605de6ad4aec9 to d02dee5ee83f3c5a35237da7afa3440f28f47307

Branch pushed to git repo; I updated commit sha1. New commits:

d02dee5typos; length_atom_rep -> n_atom_rep

comment:15 Changed 2 years ago by gh-kliem

  • 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: Changed 23 months ago by jipilab

  • 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 for H. This is what I meant originally with be consistent with Polyhedron, 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: Changed 23 months ago by 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 would vote for Vrep->Vrepresentation same for H. This is what I meant originally with be consistent with Polyhedron, 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 for ambient_Vrep(names=True)
  • make ambient_V_indices an alias for ambient_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: Changed 23 months ago by 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 for H. This is what I meant originally with be consistent with Polyhedron, I should have specified it, my bad.

In Polyhedron there is Vrepresentation and Vrep-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 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.

Got it! Looks good.

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?

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 for ambient_Vrep(names=True)
  • make ambient_V_indices an alias for ambient_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 23 months ago by gh-kliem

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 for H. This is what I meant originally with be consistent with Polyhedron, I should have specified it, my bad.

In Polyhedron there is Vrepresentation and Vrep-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 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.

Got it! Looks good.

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?

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 for ambient_Vrep(names=True)
  • make ambient_V_indices an alias for ambient_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.

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 23 months ago by git

  • Commit changed from d02dee5ee83f3c5a35237da7afa3440f28f47307 to 9d506b820f1c245b5f6ca769506d2ae58a47095e

Branch pushed to git repo; I updated commit sha1. New commits:

7b6bd9eface.Vrep(True) -> face.ambient_Vrepresentation(), face.Vrep(False) -> face.ambient_V_indices(); same for Hrep
9d506b8for CombinatorialFace: n_Vrepresentation -> n_ambient_Vrepresentation; same for Hrep

comment:21 Changed 23 months ago by gh-kliem

  • 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 23 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:23 Changed 22 months ago by gh-kliem

  • Milestone changed from sage-8.9 to sage-9.0
  • Status changed from needs_review to needs_work

I guess, we need deprecation warnings now.

comment:24 Changed 22 months ago by gh-kliem

  • Branch public/28280 deleted
  • Commit 9d506b820f1c245b5f6ca769506d2ae58a47095e deleted
  • Dependencies set to #28605
  • Description modified (diff)
  • Type changed from enhancement to task

comment:25 Changed 22 months ago by gh-kliem

  • Dependencies changed from #28605 to #28603, #28604, #28605, #28606, #28607, #28608, #28613, #28614, #28615, #28616

comment:26 Changed 22 months ago by gh-kliem

  • 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 22 months ago by gh-kliem

  • Status changed from needs_review to needs_work

comment:28 Changed 22 months ago by jipilab

  • 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 22 months ago by gh-kliem

  • Dependencies #28603, #28604, #28605, #28606, #28607, #28608, #28613, #28614, #28615, #28616 deleted
  • Description modified (diff)

comment:30 Changed 21 months ago by gh-kliem

  • Description modified (diff)

comment:31 Changed 20 months ago by gh-kliem

  • Description modified (diff)

comment:32 Changed 19 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:33 Changed 18 months ago by gh-kliem

  • Description modified (diff)

comment:34 Changed 18 months ago by gh-kliem

  • Description modified (diff)

comment:35 Changed 16 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.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 16 months ago by gh-kliem

  • Milestone changed from sage-9.2 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

This task is taken care of.

comment:37 Changed 15 months ago by jipilab

  • Cc chapoton added

Please, close this ticket as it's objectives have been fulfilled! Thanks!

comment:38 Changed 15 months ago by jipilab

  • Reviewers set to Jean-Philippe Labbé

comment:39 Changed 15 months ago by jipilab

  • Status changed from needs_review to positive_review

comment:40 Changed 15 months ago by chapoton

  • Resolution set to worksforme
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.