Three apparently useless polyhedron methods

[ncohen]

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

[mkoeppe]

_make_polyhedron_face is used in face_lattice.

[jipilab]

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

[chapoton]

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

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

[jipilab]

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

[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

[jipilab]

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

Upcoming...

[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

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

[chapoton]

Still very very broken...

[git]

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

​9f7a12b

added missing argument

[jipilab]

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

[chapoton]

Merci, feu vert.