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: | sage-9.0 |
Component: | geometry | Keywords: | |
Cc: | dimpase, vbraun, vdelecroix, chapoton | Merged in: | |
Authors: | Jean-Philippe 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 sage-6.8 to sage-duplicate/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 follow-up: ↓ 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 sage-duplicate/invalid/wontfix to sage-9.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
.