Opened 2 years ago

Closed 2 years ago

#28741 closed defect (fixed)

Lattice Polytopes: `compute_facets` does not check dimension when setting is_reflexive

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: lattice polytopes, reflexive
Cc: jipilab, gh-LaisRast, novoselt Merged in:
Authors: Jonathan Kliem Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: 769d877 (Commits, GitHub, GitLab) Commit: 769d877d2dc1b968701eb3ff8e84d4c72c356d65
Dependencies: Stopgaps:

Status badges

Description (last modified by novoselt)

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

Change History (12)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/28741
  • Cc jipilab gh-LaisRast added
  • Commit set to fbe20135708620eaa1d62ec6701706cb998ea9d1
  • Status changed from new to needs_review

New commits:

fbe2013do not overwrite `is_reflexive` in `compute_facets`

comment:2 Changed 2 years ago by novoselt

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.

comment:3 Changed 2 years ago by gh-kliem

  • Cc novoselt added

comment:4 Changed 2 years ago by gh-kliem

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.

comment:5 Changed 2 years ago by gh-kliem

But my idea isn't perfect either.

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

comment:6 Changed 2 years ago by gh-kliem

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

comment:7 Changed 2 years ago by git

  • Commit changed from fbe20135708620eaa1d62ec6701706cb998ea9d1 to 9ff978fd8e32f271c9d4610af5b85b397ec67181

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

9ff978ffix setting cache of `is_reflexive`

comment:8 Changed 2 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to positive_review

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.

comment:9 Changed 2 years ago by novoselt

  • Description modified (diff)
  • Summary changed from Lattice Polytopes: `compute_facets` has its own definition of reflexive and applies it to Lattice Polytopes: `compute_facets` does not check dimension when setting is_reflexive

comment:10 Changed 2 years ago by gh-kliem

  • Branch changed from public/28741 to public/28741-reb
  • Commit changed from 9ff978fd8e32f271c9d4610af5b85b397ec67181 to 769d877d2dc1b968701eb3ff8e84d4c72c356d65
  • Status changed from positive_review to needs_review

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:

769d877fix setting cache of `is_reflexive`

comment:11 Changed 2 years ago by novoselt

  • Status changed from needs_review to positive_review

comment:12 Changed 2 years ago by vbraun

  • Branch changed from public/28741-reb to 769d877d2dc1b968701eb3ff8e84d4c72c356d65
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.