Opened 2 years ago

Closed 2 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:

Status badges

Description (last modified by jipilab)

Preparation for #25097.

TODO: Make pynormaliz a feature.

Change History (17)

comment:1 Changed 2 years ago by mkoeppe

  • Branch set to public/normaliz-backend-refactoring

comment:2 Changed 2 years ago by git

  • Commit set to df4f20e10203fa021005d37894d03cb18ce488df

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​df4f20eRefactor normaliz backend

comment:3 Changed 2 years ago by mkoeppe

  • Status changed from new to needs_review

This branch has a part of the changes on #25097 without dependencies.

comment:4 Changed 2 years ago by vdelecroix

Do you intend to make #27682 a dependency?

comment:5 Changed 2 years ago by mkoeppe

No, not for this ticket

comment:6 Changed 2 years ago by git

  • 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: Changed 2 years ago by 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.

comment:8 in reply to: ↑ 7 Changed 2 years ago by jipilab

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 2 years ago by jipilab

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 2 years ago by jipilab

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()):
Last edited 2 years ago by slelievre (previous) (diff)

comment:11 Changed 2 years ago by git

  • Commit changed from 0dfec03083fabd664f66f296956d8452b7577a6b to 328a04a433e1b20227e296dda8bd5aea61e7660c

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

​b370268pyflakes fixes
​328a04a_nmz_result: Add doctest

comment:12 Changed 2 years ago by jipilab

  • 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 2 years ago by vbraun

  • Status changed from positive_review to needs_work 
sage -t --long src/sage/geometry/polyhedron/backend_normaliz.py  # 1 doctest failed

comment:14 Changed 2 years ago by git

  • Commit changed from 328a04a433e1b20227e296dda8bd5aea61e7660c to ee5ff410ee028c8563406c2a02cf279051ad95fb

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

​ee5ff41Mark newest doctest # optional - pynormaliz

comment:15 Changed 2 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:16 Changed 2 years ago by jipilab

  • Status changed from needs_review to positive_review

LGTM now hopefully no more doctests failure.

comment:17 Changed 2 years ago by vbraun

  • Branch changed from public/normaliz-backend-refactoring to ee5ff410ee028c8563406c2a02cf279051ad95fb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.