Opened 3 years ago
Closed 3 years ago
#27716 closed enhancement (fixed)
Refactor backend_normaliz
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  geometry  Keywords:  
Cc:  vdelecroix, jipilab  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  ee5ff41 (Commits, GitHub, GitLab)  Commit:  ee5ff410ee028c8563406c2a02cf279051ad95fb 
Dependencies:  Stopgaps: 
Description (last modified by )
Preparation for #25097.
TODO: Make pynormaliz a feature.
Change History (17)
comment:1 Changed 3 years ago by
 Branch set to public/normalizbackendrefactoring
comment:2 Changed 3 years ago by
 Commit set to df4f20e10203fa021005d37894d03cb18ce488df
comment:3 Changed 3 years ago by
 Status changed from new to needs_review
This branch has a part of the changes on #25097 without dependencies.
comment:4 Changed 3 years ago by
Do you intend to make #27682 a dependency?
comment:5 Changed 3 years ago by
No, not for this ticket
comment:6 Changed 3 years ago by
 Commit changed from df4f20e10203fa021005d37894d03cb18ce488df to 0dfec03083fabd664f66f296956d8452b7577a6b
Branch pushed to git repo; I updated commit sha1. New commits:
0dfec03  _format_function_call: Fix doctest

comment:7 followup: ↓ 8 Changed 3 years ago by
I think in #27682 there is a need to touch the normaliz backend due to some changed in the (not yet released) PyNormaliz. So there might be a small conflict.
comment:8 in reply to: ↑ 7 Changed 3 years ago by
Replying to vdelecroix:
I think in #27682 there is a need to touch the normaliz backend due to some changed in the (not yet released) PyNormaliz. So there might be a small conflict.
Yes, Matthias merged cleanly so far. It's coming...
comment:9 Changed 3 years ago by
Here are a few things:
 to get 100% coverage, the function
_nmz_result
should be tested. Perhaps also with a function name that does not exist?  sage.matrix.constructor.matrix imported but not used
 pep8 around here, and blankline at end of file...........
921 if box_points<threshold: 922 from sage.geometry.integral_points import rectangular_box_points 923 return rectangular_box_points(list(box_min), list(box_max), self)
Otherwise, it looks quite good, let's see what the bot says. vdelecroix mentioned in #25091 in comment https://trac.sagemath.org/ticket/25091#comment:69 that
import PyNormaliz
is not desirable... and we should use feature. I would not do this here but in a follow up ticket.
comment:10 Changed 3 years ago by
There seems to be only two small things:
Pyflakes:
+src/sage/geometry/polyhedron/backend_normaliz.py:33: 'sage.matrix.constructor.matrix' imported but unused + +src/sage/geometry/polyhedron/parent.py:21: 'sage.geometry.polyhedron.base.Polyhedron_base' imported but unused
Python3:
+inside file: b/src/sage/geometry/polyhedron/backend_normaliz.py +@@ 496,25 +607,36 @@ class Polyhedron_normaliz(Polyhedron_base): ++ for key, value in sorted(data.iteritems()):
comment:11 Changed 3 years ago by
 Commit changed from 0dfec03083fabd664f66f296956d8452b7577a6b to 328a04a433e1b20227e296dda8bd5aea61e7660c
comment:12 Changed 3 years ago by
 Description modified (diff)
 Reviewers set to JeanPhilippe Labbé
 Status changed from needs_review to positive_review
LGTM. The bot gave the green light and I am satisfied.
A next step would be to make pynormaliz a feature...
comment:13 Changed 3 years ago by
 Status changed from positive_review to needs_work
sage t long src/sage/geometry/polyhedron/backend_normaliz.py # 1 doctest failed
comment:14 Changed 3 years ago by
 Commit changed from 328a04a433e1b20227e296dda8bd5aea61e7660c to ee5ff410ee028c8563406c2a02cf279051ad95fb
Branch pushed to git repo; I updated commit sha1. New commits:
ee5ff41  Mark newest doctest # optional  pynormaliz

comment:15 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 3 years ago by
 Status changed from needs_review to positive_review
LGTM now hopefully no more doctests failure.
comment:17 Changed 3 years ago by
 Branch changed from public/normalizbackendrefactoring to ee5ff410ee028c8563406c2a02cf279051ad95fb
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Refactor normaliz backend