Lattice Polytopes: compute_facets does not check dimension when setting is_reflexive

[kliem]

Currently, computing facets of a lattice polytope, changes whether it is reflexive or not.

sage: p = LatticePolytope([], lattice=ToricLattice(3).dual()); p
-1-d lattice polytope in 3-d lattice M
sage: a = p.faces()[0][0]
sage: p = LatticePolytope([], lattice=ToricLattice(3).dual()); p
-1-d lattice polytope in 3-d lattice M
sage: a = p.faces()[0][0]; a
-1-d lattice polytope in 3-d lattice M
sage: a.facet_normals()
Empty collection
in 3-d lattice N
sage: a
-1-d reflexive polytope in 3-d lattice M

CC: @jplab @LaisRast @novoselt

Component: geometry

Keywords: lattice polytopes, reflexive

Author: Jonathan Kliem

Branch/Commit: 769d877

Reviewer: Andrey Novoseltsev

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

[kliem]

New commits:

fbe2013

do not overwrite `is_reflexive` in `compute_facets`

[kliem]

Branch: public/28741

[kliem]

Commit: fbe2013

[novoselt]

comment:2

What do you mean its own definition??? It is the same definition as in other places and it does not overwrite is_reflexive but rather sets a cached value. However the code failed to check that the polytope is full-dimensional, so I'd much rather keep the old code and add the missing check rather than getting rid of an easy computation and triggering another one right away with is_reflexive() call.

[kliem]

comment:4

Sorry. I was completely puzzled about this and exaggerated.

As you can probably tell, I stumbled open this trying to implement incidence matrix.

I still don't get the advantage of the current setup. Is it because we avoid making a copy of the constants?

What currently happens with is_reflexive is that it implicitly calls compute_facets, which computes the value for is_reflexive. However, is_reflexive now recomputes the value and returns it. I don't think this is ideal.

[kliem]

comment:5

But my idea isn't perfect either.

It is still double calculation. Just executing the same code now.

[kliem]

comment:6

Anyway. I don't have hard feelings about this. I think we can just fix it in place.

[sagetrac-git]

Changed commit from fbe2013 to 9ff978f

[sagetrac-git]

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

9ff978f

fix setting cache of `is_reflexive`

[novoselt]

Reviewer: Andrey Novoseltsev

[novoselt]

comment:8

Sorry for confusing code, believe me it used to be even more convoluted, especially with regard to determining and using reflexivity ;-) It still feels to me that this is a better fix.

[kliem]

Changed commit from 9ff978f to 769d877

[kliem]

comment:10

Had to rebase.

This time I did not delete any trailing spaces etc. Implementing the incidence matrix in #28743 will take care of that.

---

New commits:

769d877

fix setting cache of `is_reflexive`

[kliem]

Changed branch from public/28741 to public/28741-reb

[vbraun]

Changed branch from public/28741-reb to 769d877