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:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  lattice polytopes, reflexive 
Cc:  jipilab, ghLaisRast, novoselt  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Andrey Novoseltsev 
Report Upstream:  N/A  Work issues:  
Branch:  769d877 (Commits, GitHub, GitLab)  Commit:  769d877d2dc1b968701eb3ff8e84d4c72c356d65 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently, computing facets of a lattice polytope, changes whether it is reflexive or not.
sage: p = LatticePolytope([], lattice=ToricLattice(3).dual()); p 1d lattice polytope in 3d lattice M sage: a = p.faces()[0][0] sage: p = LatticePolytope([], lattice=ToricLattice(3).dual()); p 1d lattice polytope in 3d lattice M sage: a = p.faces()[0][0]; a 1d lattice polytope in 3d lattice M sage: a.facet_normals() Empty collection in 3d lattice N sage: a 1d reflexive polytope in 3d lattice M
Change History (12)
comment:1 Changed 2 years ago by
 Branch set to public/28741
 Cc jipilab ghLaisRast added
 Commit set to fbe20135708620eaa1d62ec6701706cb998ea9d1
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
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 fulldimensional, 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
 Cc novoselt added
comment:4 Changed 2 years ago by
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
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
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
 Commit changed from fbe20135708620eaa1d62ec6701706cb998ea9d1 to 9ff978fd8e32f271c9d4610af5b85b397ec67181
Branch pushed to git repo; I updated commit sha1. New commits:
9ff978f  fix setting cache of `is_reflexive`

comment:8 Changed 2 years ago by
 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
 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
 Branch changed from public/28741 to public/28741reb
 Commit changed from 9ff978fd8e32f271c9d4610af5b85b397ec67181 to 769d877d2dc1b968701eb3ff8e84d4c72c356d65
 Status changed from positive_review to needs_review
comment:11 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:12 Changed 2 years ago by
 Branch changed from public/28741reb to 769d877d2dc1b968701eb3ff8e84d4c72c356d65
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
do not overwrite `is_reflexive` in `compute_facets`