Opened 20 months ago
Closed 20 months ago
#28379 closed enhancement (fixed)
clean normaliz backend
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  geometry  Keywords:  
Cc:  jipilab, mkoeppe  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  c19b548 (Commits, GitHub, GitLab)  Commit:  c19b548ddbf490041af7fb1e4577fb1b873091b4 
Dependencies:  Stopgaps: 
Description (last modified by )
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
 Branch set to u/vdelecroix/28379
 Commit set to c78d9a6b24634c2b2e1e8633b83fc013b39364ae
 Status changed from new to needs_review
comment:2 Changed 20 months ago by
 Description modified (diff)
comment:3 Changed 20 months ago by
 Reviewers set to JeanPhilippe 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
 Description modified (diff)
 Summary changed from clean normaliz interface to clean normaliz backend
comment:5 followup: ↓ 6 Changed 20 months ago by
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 ; followup: ↓ 9 Changed 20 months ago by
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.
comment:7 Changed 20 months ago by
 Commit changed from c78d9a6b24634c2b2e1e8633b83fc013b39364ae to c19b548ddbf490041af7fb1e4577fb1b873091b4
Branch pushed to git repo; I updated commit sha1. New commits:
c19b548  fix py2 output

comment:8 Changed 20 months ago by
 Status changed from needs_work to needs_review
comment:9 in reply to: ↑ 6 Changed 20 months ago by
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 argumentnormaliz_cone
... which means that it is never called ifPyNormaliz
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/sagepatchbot/.sage/temp/rk02math/14911/dir_e02POx/preview.png', see '/local/sagepatchbot/.sage/temp/rk02math/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
 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
 Branch changed from u/vdelecroix/28379 to c19b548ddbf490041af7fb1e4577fb1b873091b4
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
clean normaliz interface