Three apparently useless polyhedron methods

[nathanncohen]

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

CC: @dimpase @vbraun @videlec @fchapoton

Component: geometry

Author: Jean-Philippe Labbé

Branch/Commit: 9f7a12b

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/18861

[mkoeppe]

comment:1

_make_polyhedron_face is used in face_lattice.

[jplab]

comment:2

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".

[fchapoton]

comment:4

Maybe _make_polyhedron_face could just be inserted where it is called, as it is very short and called in just one place ?

[jplab]

comment:5

Replying to @fchapoton:

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.

[jplab]

comment:6

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

[jplab]

Commit: a7792fa

[jplab]

Author: Jean-Philippe Labbé

[jplab]

Branch: u/jipilab/18861

[fchapoton]

comment:7

• 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

[jplab]

comment:8

Merde. I thought about it when I cut the function, and after I pasted, I forgot to change it's format.

Upcoming...

[sagetrac-git]

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

dc629e0	removed _make_polyhedron_face
542d772	changed if in face_lattice

[sagetrac-git]

Changed commit from a7792fa to 542d772

[jplab]

comment:10

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...

[fchapoton]

comment:11

Still very very broken...

[sagetrac-git]

Changed commit from 542d772 to 9f7a12b

[sagetrac-git]

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

9f7a12b

added missing argument

[jplab]

comment:13

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

[fchapoton]

Reviewer: Frédéric Chapoton

[fchapoton]

comment:14

Merci, feu vert.

[vbraun]

Changed branch from u/jipilab/18861 to 9f7a12b