Opened 3 years ago

Closed 3 years 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 3 years 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 3 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 3 years 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 3 years ago by jipilab

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

comment:5 follow-up: Changed 3 years 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 3 years 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 3 years ago by vdelecroix (previous) (diff)

comment:7 Changed 3 years 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 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:9 in reply to: ↑ 6 Changed 3 years 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 3 years 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 3 years 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.