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:

Status badges

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 mkoeppe

_make_polyhedron_face is used in face_lattice.

comment:2 Changed 18 months ago by jipilab

  • 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 jipilab

  • Status changed from new to needs_review

comment:4 follow-up: Changed 18 months ago by chapoton

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 jipilab

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 jipilab

  • Authors set to Jean-Philippe Labbé
  • 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:

a7792faremoved _make_polyhedron_face

comment:7 Changed 18 months ago by chapoton

  • 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 jipilab

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 git

  • Commit changed from a7792fa237026eaf9c4369999d294f72b923dc14 to 542d772958bb6e6dd2a4c2b4656672ac76444ca2

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

dc629e0removed _make_polyhedron_face
542d772changed if in face_lattice

comment:10 Changed 18 months ago by jipilab

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 the face_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 chapoton

Still very very broken...

comment:12 Changed 18 months ago by git

  • Commit changed from 542d772958bb6e6dd2a4c2b4656672ac76444ca2 to 9f7a12bb908fdb0ffba1cc110a93e84645d3cbc2

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

9f7a12badded missing argument

comment:13 Changed 18 months ago by jipilab

Wow, yes... I missed that self argument... Should be better now...

comment:14 Changed 18 months ago by chapoton

  • 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 vbraun

  • Branch changed from u/jipilab/18861 to 9f7a12bb908fdb0ffba1cc110a93e84645d3cbc2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.