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: |
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 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
- Description modified (diff)
- Summary changed from clean normaliz interface to clean normaliz backend
comment:5 follow-up: ↓ 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 ; follow-up: ↓ 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/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
- 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