Opened 6 years ago
Closed 18 months ago
#18861 closed enhancement (fixed)
Three apparently useless polyhedron methods
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  
Cc:  dimpase, vbraun, vdelecroix, chapoton  Merged in:  
Authors:  JeanPhilippe Labbé  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  9f7a12b (Commits, GitHub, GitLab)  Commit:  9f7a12bb908fdb0ffba1cc110a93e84645d3cbc2 
Dependencies:  Stopgaps: 
Description
It seems that the three following functions are not used anywhere
(polgraph✚2…)~/sage/geometry$ grep _make_polyh . R ./polyhedron/base.py:# _make_polyhedron_face. ./polyhedron/base.py: def _make_polyhedron_face(self, Vindices, Hindices): ./polyhedron/base.py: sage: square._make_polyhedron_face((0,2), (1,)) ./polyhedron/base.py: return self._make_polyhedron_face(Vindices, Hindices) (polgraph✚2…)~/sage/geometry$ grep _init_fac . R ./polyhedron/base.py:# _init_facet_adjacency_matrix, _init_vertex_adjacency_matrix, and ./polyhedron/backend_cdd.py: def _init_facet_adjacency_matrix(self, verbose=False): ./polyhedron/backend_cdd.py: sage: p._init_facet_adjacency_matrix() (polgraph✚2…)~/sage/geometry$ grep _init_vertex . R ./polyhedron/base.py:# _init_facet_adjacency_matrix, _init_vertex_adjacency_matrix, and ./polyhedron/backend_cdd.py: def _init_vertex_adjacency_matrix(self, verbose=False): ./polyhedron/backend_cdd.py: sage: p._init_vertex_adjacency_matrix()
Should they be removed, or used somewhere?
Nathann
Change History (15)
comment:1 Changed 5 years ago by
comment:2 Changed 18 months ago by
 Cc chapoton added
 Milestone changed from sage6.8 to sageduplicate/invalid/wontfix
With sage8.9:
sage/src/sage/geometry$ grep _make_polyh . R ./polyhedron/base.py:# _make_polyhedron_face. ./polyhedron/base.py: def _make_polyhedron_face(self, Vindices, Hindices): ./polyhedron/base.py: sage: square._make_polyhedron_face((0,2), (1,)).ambient_V_indices() ./polyhedron/base.py: return self._make_polyhedron_face(Vindices, Hindices) sage/src/sage/geometry$ grep _init_fac . R sage/src/sage/geometry$ grep _init_vertex . R
This has been indirectly taken care of. The first function is used in face_lattice
.
For this reason, I would set this as a "won't fix".
comment:3 Changed 18 months ago by
 Status changed from new to needs_review
comment:4 followup: ↓ 5 Changed 18 months ago by
Maybe _make_polyhedron_face
could just be inserted where it is called, as it is very short and called in just one place ?
comment:5 in reply to: ↑ 4 Changed 18 months ago by
Replying to chapoton:
Maybe
_make_polyhedron_face
could just be inserted where it is called, as it is very short and called in just one place ?
Good idea. I'll do that.
comment:6 Changed 18 months ago by
 Branch set to u/jipilab/18861
 Commit set to a7792fa237026eaf9c4369999d294f72b923dc14
 Milestone changed from sageduplicate/invalid/wontfix to sage9.0
 Type changed from defect to enhancement
It turns out that it can simply be deleted and merged into the already present function.
... when face_lattice
will be renovated through CombinatorialPolyhedron? the face lattice function will change again, but it will make the transition a bit easier. So I would say it is a good first step.
New commits:
a7792fa  removed _make_polyhedron_face

comment:7 Changed 18 months ago by
 you forgot the first argument
self
in the function call.
 I would put the import outside of the auxiliary function
 One could replace
if len(atoms) == 0:
by
if not atoms
comment:8 Changed 18 months ago by
Merde. I thought about it when I cut the function, and after I pasted, I forgot to change it's format.
Upcoming...
comment:9 Changed 18 months ago by
 Commit changed from a7792fa237026eaf9c4369999d294f72b923dc14 to 542d772958bb6e6dd2a4c2b4656672ac76444ca2
comment:10 Changed 18 months ago by
I looked at it and I think that the first two items are not correct:
what happened is
 the method
_make_polyhedron_face
is removed.  the only place where it was used was related to an internally defined function
face_constructor
inside of theface_lattice
method.
So, I believe that the import should be inside the internal function, right?
Thus, there is no need of the self argument.
I changed the if statement. Let's see what the bot says, just to make sure that the calls are correct. If I looked correctly, it should essentially be the same, but only function call less...
comment:11 Changed 18 months ago by
Still very very broken...
comment:12 Changed 18 months ago by
 Commit changed from 542d772958bb6e6dd2a4c2b4656672ac76444ca2 to 9f7a12bb908fdb0ffba1cc110a93e84645d3cbc2
Branch pushed to git repo; I updated commit sha1. New commits:
9f7a12b  added missing argument

comment:13 Changed 18 months ago by
Wow, yes... I missed that self
argument... Should be better now...
comment:14 Changed 18 months ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
Merci, feu vert.
comment:15 Changed 18 months ago by
 Branch changed from u/jipilab/18861 to 9f7a12bb908fdb0ffba1cc110a93e84645d3cbc2
 Resolution set to fixed
 Status changed from positive_review to closed
_make_polyhedron_face
is used inface_lattice
.