Opened 20 months ago

Closed 20 months ago

#28379 closed enhancement (fixed)

clean normaliz backend

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.9
Component: geometry Keywords:
Cc: jipilab, mkoeppe Merged in:
Authors: Vincent Delecroix Reviewers: Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: c19b548 (Commits, GitHub, GitLab) Commit: c19b548ddbf490041af7fb1e4577fb1b873091b4
Dependencies: Stopgaps:

Status badges

Description (last modified by jipilab)

We simplify the normaliz backend. A lot of code is not needed anymore on the last version of PyNormaliz.

Change History (11)

comment:1 Changed 20 months ago by vdelecroix

  • Branch set to u/vdelecroix/28379
  • Commit set to c78d9a6b24634c2b2e1e8633b83fc013b39364ae
  • Status changed from new to needs_review

New commits:

​c78d9a6clean normaliz interface

comment:2 Changed 20 months ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 20 months ago by jipilab

  • Reviewers set to Jean-Philippe Labbé
  • Status changed from needs_review to needs_work

2 of the new doctests are failing.

They are still python integers for some reason?

comment:4 Changed 20 months ago by jipilab

  • Description modified (diff)
  • Summary changed from clean normaliz interface to clean normaliz backend

comment:5 follow-up: Changed 20 months ago by jipilab

A question:

Removing the following line is ok? 

- PythonModule("PyNormaliz", spkg="pynormaliz").require()

Wasn't it the way to should be? Or it would be caught earlier in the call/the creation of the object?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 20 months ago by vdelecroix

Replying to jipilab:

A question:

Removing the following line is ok? 

- PythonModule("PyNormaliz", spkg="pynormaliz").require()

Wasn't it the way to should be? Or it would be caught earlier in the call/the creation of the object?

It is the way it should be done, but it is not the place it should be done. The function _nmz_result has as first argument normaliz_cone... which means that it is never called if PyNormaliz is not present. Checking for its presence inside the function is very weird.

Last edited 20 months ago by vdelecroix (previous) (diff)

comment:7 Changed 20 months ago by git

  • Commit changed from c78d9a6b24634c2b2e1e8633b83fc013b39364ae to c19b548ddbf490041af7fb1e4577fb1b873091b4

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

​c19b548fix py2 output

comment:8 Changed 20 months ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:9 in reply to: ↑ 6 Changed 20 months ago by jipilab

Replying to vdelecroix:

Replying to jipilab:

A question:

Removing the following line is ok? 

- PythonModule("PyNormaliz", spkg="pynormaliz").require()

Wasn't it the way to should be? Or it would be caught earlier in the call/the creation of the object?

It is the way it should be done, but it is not the place it should be done.

ok! good to know.

The function _nmz_result has as first argument normaliz_cone... which means that it is never called if PyNormaliz is not present. Checking for its presence inside the function is very weird.

Yep, I see. I probably forgot that instance when I changed the import...

In the bot, there is one doctest warning in platonic.py that gives one doctest failure. 

sage -t --long src/sage/plot/plot3d/platonic.py
**********************************************************************
File "src/sage/plot/plot3d/platonic.py", line 211, in sage.plot.plot3d.platonic.tetrahedron
Failed example:
    tetrahedron(aspect_ratio=[1,1,1]).scale((4,4,1))
Expected:
    Graphics3d Object
Got:
    doctest:warning
    [snip]
    RichReprWarning: Exception in _rich_repr_ while displaying object: Jmol failed to create file '/local/sage-patchbot/.sage/temp/rk02-math/14911/dir_e02POx/preview.png', see '/local/sage-patchbot/.sage/temp/rk02-math/14911/tmp_XfwJq0.txt' for details
    Graphics3d Object
**********************************************************************
1 item had failures:
   1 of  15 in sage.plot.plot3d.platonic.tetrahedron
    [52 tests, 1 failure, 6.60 s]

It does not seem to have anything to do with this ticket though... Hmm.

comment:10 Changed 20 months ago by jipilab

  • Status changed from needs_review to positive_review

The error in platonic.py was not present in the last bot test... I would say it is ready to go.

comment:11 Changed 20 months ago by vbraun

  • Branch changed from u/vdelecroix/28379 to c19b548ddbf490041af7fb1e4577fb1b873091b4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.