Opened 12 months ago
Closed 11 months ago
#29569 closed enhancement (fixed)
Obtain polar with both Vrep and Hrep (if backend supports it)
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  geometry  Keywords:  polar, polytopes 
Cc:  jipilab, ghLaisRast  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  c4f9dba (Commits, GitHub, GitLab)  Commit:  c4f9dbac6a8eeb3a77384a46fd06d9ce0ffdc3ee 
Dependencies:  #29568  Stopgaps: 
Description (last modified by )
We obtain the polar with both Vrep and Hrep to speed things up.
Along the way we optimize translation
a bit in the spirit of #28866 and we outsource obtaining the new double description from this method.
Now one can obtain the new data with _translation_double_description
without actually creating anything.
Change History (21)
comment:1 Changed 12 months ago by
 Description modified (diff)
comment:2 Changed 12 months ago by
 Branch set to public/29569
 Commit set to 69513eb0e33d8b4ba3426b067f76dae784edf0f5
comment:3 Changed 12 months ago by
 Description modified (diff)
comment:4 Changed 12 months ago by
 Milestone changed from sage9.1 to sage9.2
Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:5 Changed 12 months ago by
 Branch changed from public/29569 to public/29569reb
 Commit changed from 69513eb0e33d8b4ba3426b067f76dae784edf0f5 to fc0a37571cc261c126ec6550e891e04189a80811
 Status changed from new to needs_review
comment:6 Changed 11 months ago by
 Branch changed from public/29569reb to public/29569reb2
 Commit changed from fc0a37571cc261c126ec6550e891e04189a80811 to 758902c944cc17207f953298afad238856bb1804
New commits:
7e5ecaf  initalize cdd with both Hrep and Vrep to deal with numerical inconsistency

6cedfb6  get only the new double descpription from translation

803b6ac  documentation for `_translation_double_description`

3e4ab32  use precomputed data for polar

8bbd5cb  fix doctets

a39af8b  fixed failing doctest

758902c  removed redundant import

comment:7 followup: ↓ 9 Changed 11 months ago by
About the bug that you mentioned, it is really a bug?
I am asking because "polar" means different things depending on which corner of the street you stand at...
sage: p = LatticePolytope([[1,0],[0,1],[1,1]]) sage: p 2d reflexive polytope #0 in 2d lattice M sage: p.polar() 2d reflexive polytope #15 in 2d lattice N sage: p.polar().vertices() N( 2, 1), N(1, 2), N(1, 1) in 2d lattice N
comment:8 Changed 11 months ago by
 Status changed from needs_review to needs_work
In the tutorial, it would be good to still have an example where it really still gives an error. There is likely such an example in the library (see those that are forced to be exact).
I would then keep a error and a warning in the tutorial to maintain an uptodate document.
comment:9 in reply to: ↑ 7 Changed 11 months ago by
Replying to jipilab:
About the bug that you mentioned, it is really a bug?
I am asking because "polar" means different things depending on which corner of the street you stand at...
sage: p = LatticePolytope([[1,0],[0,1],[1,1]]) sage: p 2d reflexive polytope #0 in 2d lattice M sage: p.polar() 2d reflexive polytope #15 in 2d lattice N sage: p.polar().vertices() N( 2, 1), N(1, 2), N(1, 1) in 2d lattice N
Thanks for checking this. No it's not a bug. It's exactly the output described in OUTPUT
.
It's still confusing. But this ticket shouldn't be about making things more uniform.
comment:10 Changed 11 months ago by
 Description modified (diff)
comment:11 Changed 11 months ago by
 Commit changed from 758902c944cc17207f953298afad238856bb1804 to 81cf70f32da929c2d6b8dba5756bd936e4ad0072
comment:12 Changed 11 months ago by
 Status changed from needs_work to needs_review
comment:13 Changed 11 months ago by
The fix in the last commit means that it was no passing the tests? It would also be nice to have the "warning" case and the "error" case if we can find the two in the library for example.
comment:14 Changed 11 months ago by
If you sort it, you get an error. If you don't sort it, you get a warning.
comment:15 Changed 11 months ago by
 Commit changed from 81cf70f32da929c2d6b8dba5756bd936e4ad0072 to c4f9dbac6a8eeb3a77384a46fd06d9ce0ffdc3ee
Branch pushed to git repo; I updated commit sha1. New commits:
c4f9dba  give both examples: Error and Warning

comment:16 Changed 11 months ago by
Now I'm giving both examples.
comment:17 Changed 11 months ago by
 Reviewers set to JeanPhilippe Labbé
 Status changed from needs_review to positive_review
Looks good to me now. The code styles are fixed in a different ticket.
comment:18 Changed 11 months ago by
Thank you.
comment:19 followup: ↓ 20 Changed 11 months ago by
@jipilab: Are you ware that the first commit of this ticket is actually #29568.
comment:20 in reply to: ↑ 19 Changed 11 months ago by
comment:21 Changed 11 months ago by
 Branch changed from public/29569reb2 to c4f9dbac6a8eeb3a77384a46fd06d9ce0ffdc3ee
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
get only the new double descpription from translation
documentation for `_translation_double_description`
use precomputed data for polar