Opened 3 years ago
Closed 3 years ago
#27716 closed enhancement (fixed)
Refactor backend_normaliz
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.8 |
Component: | geometry | Keywords: | |
Cc: | vdelecroix, jipilab | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | Jean-Philippe 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/normaliz-backend-refactoring
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 follow-up: ↓ 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 Jean-Philippe 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/normaliz-backend-refactoring 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