Opened 2 years ago

Closed 22 months ago

#28605 closed enhancement (fixed)

CombinatorialPolyhedron: replace attributes by methods

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: polytopes, combinatorial polyhedron
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Laith Rastanawi, Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: d4b3163 (Commits, GitHub, GitLab) Commit: d4b3163762932706ba2af5493e55c4d4c4a58ed0
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

Replace attributes in CombinatorialPolyhedron by methods, such that they can potentially be lazily evaluated.

More precisely we replace an attribute by a private attribute (with leading _) and add a method without leading _. E.g. the attribute far_face_tuple is replaced by _far_face_tuple and we add a method far_face_tuple(self). Thus we gain flexibility in the sense that those attributes must not be set on initialization.

This is motivated by #10777.

We remove the attribute Vinv completely, as it is not being used.

Change History (21)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/28605
  • Commit set to 37592f9458a1f680f90c6befd4b238558b8d5721

New commits:

d597ed3replace attributes by methods
2fc4fe0removed empty folder being created in source
37592f9replace attributes by methods; remove empty folder from source

comment:2 Changed 2 years ago by gh-kliem

  • Keywords polytopes combinatorial polyhedron added

comment:3 Changed 2 years ago by gh-kliem

  • Description modified (diff)

comment:4 Changed 2 years ago by git

  • Commit changed from 37592f9458a1f680f90c6befd4b238558b8d5721 to 588afa4cf3b700e142de154f97d32ee33795b6a9

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

e865f9dremoved attribute Vinv, as its not being used
84ef31badded docstrings to the new methods
588afa4removed method for Vinv

comment:5 Changed 2 years ago by gh-kliem

  • Cc jipilab gh-LaisRast added
  • Status changed from new to needs_review

comment:6 Changed 2 years ago by gh-kliem

  • Dependencies set to #28625

Adding #28625 as I'm using attributes in that ticket.

At moment I think #28625 might be finished before this here, but we can always revers dependencies and change it there.

comment:7 Changed 2 years ago by gh-LaisRast

  • Reviewers set to Laith Rastanawi
  • Status changed from needs_review to positive_review

I went through the changes and they seem all legit. So I will put this on "positive review".

comment:8 Changed 2 years ago by git

  • Commit changed from 588afa4cf3b700e142de154f97d32ee33795b6a9 to 9b5bcaafd61760a75f47451df5febf91d55cff12
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

b89610eadded combinatorial polyhedron as an attribute for polyhedra
326602cf_vector of CombinatorialPolyhedron is a vector
dfbe2adMerge branch 'public/28607' of git://trac.sagemath.org/sage into public/28621
ed5518bused CombinatorialPolyhedron to compute f_vector
9bdd005give an error message for polytopes in some cases; removed incorrect example
acd671dnow we get a precice error message for inexact truncated dodecahedron
bf85a62subsequent calls for f_vector fail if first attempt fails
dc99ea4Merge branch 'public/28625' of git://trac.sagemath.org/sage into public/28605
9b5bcaaapplied changes of 28605 to new code from 28625

comment:9 Changed 2 years ago by gh-LaisRast

  • Status changed from needs_review to positive_review

comment:10 Changed 2 years ago by gh-kliem

  • Status changed from positive_review to needs_work

Failing doctest. Removing .cc to get rid of the empty folder doesn't seem to work.

Should probably just do this in an extra ticket and see what happens then.

comment:11 Changed 2 years ago by git

  • Commit changed from 9b5bcaafd61760a75f47451df5febf91d55cff12 to 473b15d1f08eca87314a201477dcc1e2bd286b7c

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

473b15dundid change to module list

comment:12 Changed 2 years ago by gh-kliem

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

I think we should to the change to src/module_list.py in another ticket. There is no need to do it now, especially if it leads to problems.

comment:13 Changed 2 years ago by git

  • Commit changed from 473b15d1f08eca87314a201477dcc1e2bd286b7c to 4f49eac6517be9e46924a6368446ac3697cbd46a

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

202cad9replace attributes by methods
2e3e464removed empty folder being created in source
7701062removed attribute Vinv, as its not being used
1b17f6eadded docstrings to the new methods
c51cdd9removed method for Vinv
dd21f9capplied changes of 28605 to new code from 28625
4f49eacundid change to module list

comment:14 Changed 2 years ago by gh-kliem

  • Description modified (diff)

comment:15 Changed 23 months ago by gh-kliem

  • Branch changed from public/28605 to public/28605-reb
  • Commit changed from 4f49eac6517be9e46924a6368446ac3697cbd46a to 0f3b121b9f1cebe0c65056bb2fc364e9b770e58f

New commits:

0f3b121replace attributes by methods; remove attribute Vinv

comment:16 Changed 23 months ago by gh-kliem

  • Dependencies #28625 deleted

comment:17 Changed 23 months ago by git

  • Commit changed from 0f3b121b9f1cebe0c65056bb2fc364e9b770e58f to b0eac0858f113c04beb123e747851982d6447a65

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

b0eac08replace attributes by methods; remove attribute Vinv

comment:18 Changed 23 months ago by jipilab

  • Status changed from needs_review to needs_work

Failing doctests on beta5.

comment:19 Changed 23 months ago by gh-kliem

  • Branch changed from public/28605-reb to public/28605-reb2
  • Commit changed from b0eac0858f113c04beb123e747851982d6447a65 to d4b3163762932706ba2af5493e55c4d4c4a58ed0
  • Status changed from needs_work to needs_review

New commits:

dd0729dreplace attributes by methods; remove attribute Vinv
d4b3163small fix in ridges

comment:20 Changed 23 months ago by jipilab

  • Reviewers changed from Laith Rastanawi to Laith Rastanawi, Jean-Philippe Labbé
  • Status changed from needs_review to positive_review

Ok! Looks good to me now.

comment:21 Changed 22 months ago by vbraun

  • Branch changed from public/28605-reb2 to d4b3163762932706ba2af5493e55c4d4c4a58ed0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.