Opened 7 years ago
Closed 6 years ago
#18057 closed enhancement (fixed)
Cython interface to libhomfly
Reported by:  mmarco  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  interfaces: optional  Keywords:  days74 
Cc:  vbraun, kcrisman, amitjadagni, fugelde, tscrim  Merged in:  
Authors:  Miguel Marco  Reviewers:  Jeroen Demeyer, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  cd786cb (Commits, GitHub, GitLab)  Commit:  cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1 
Dependencies:  Stopgaps: 
Description
This implements a cython interface to the libhomfly library in #18047
Change History (28)
comment:1 Changed 7 years ago by
 Branch set to u/mmarco/libhomfly_interface
comment:2 Changed 7 years ago by
 Commit set to d3e96fc26750c34166cff23b64a706ebc6260b63
 Component changed from interfaces to interfaces: optional
comment:3 Changed 6 years ago by
 Cc fugelde tscrim added
comment:4 Changed 6 years ago by
 Commit changed from d3e96fc26750c34166cff23b64a706ebc6260b63 to 69ec550c0ff5ffc526b0ceb06bc899e166221f2f
comment:5 Changed 6 years ago by
 Keywords days74 added
 Status changed from new to needs_review
comment:6 Changed 6 years ago by
 Commit changed from 69ec550c0ff5ffc526b0ceb06bc899e166221f2f to 2019b9d67621ece7f830a8f20e6d568af6dc8713
Branch pushed to git repo; I updated commit sha1. New commits:
2019b9d  Moved dependency to cysignals

comment:7 Changed 6 years ago by
 Status changed from needs_review to needs_work
Delete this commentedout code:
#clib homfly #clib gc
comment:8 Changed 6 years ago by
You should use OptionalExtension
, see other examples in module_list.py
.
comment:9 Changed 6 years ago by
 Commit changed from 2019b9d67621ece7f830a8f20e6d568af6dc8713 to 53ad4bffbcfe4d5ee5f13b7a5383c15d5b9e858b
Branch pushed to git repo; I updated commit sha1. New commits:
53ad4bf  Adapted to the new interface with functions for strings and dict

comment:10 Changed 6 years ago by
 Dependencies #18047 deleted
This technically does not depend on #18047.
comment:11 Changed 6 years ago by
The resulting Poly
object was created under GC_malloc
, do we have to worry about deleting it after we are done translating it into a dict
?
comment:12 Changed 6 years ago by
I think you are right, we should free it.
comment:13 followup: ↓ 14 Changed 6 years ago by
Wait, it was created by malloc or by boehm's version of malloc? If it is the latter, the garbage collector should take care of that.
comment:14 in reply to: ↑ 13 Changed 6 years ago by
Replying to mmarco:
Wait, it was created by malloc or by boehm's version of malloc? If it is the latter, the garbage collector should take care of that.
It was the latter (i.e., the memory was given by a call to GC_MALLOC
). Although I think we are okay in doing the freeing ourselves.
comment:15 Changed 6 years ago by
 Branch changed from u/mmarco/libhomfly_interface to u/tscrim/libhomfly_cython_interface18057
 Commit changed from 53ad4bffbcfe4d5ee5f13b7a5383c15d5b9e858b to eeda42566c0e90abe38ee9f7669516a647079c68
 Milestone changed from sage6.6 to sage7.3
 Reviewers set to Jeroen Demeyer, Travis Scrimshaw
I've made this OptionalExtension
and removed the commented out code.
New commits:
eeda425  Removing commented out code and making OptionalExtension.

comment:16 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 6 years ago by
 Status changed from needs_review to needs_work
When libhomfly
is not installed:
sage t src/sage/libs/homfly.pyx ********************************************************************** File "src/sage/libs/homfly.pyx", line 65, in sage.libs.homfly.homfly_polynomial_string Failed example: from sage.libs.homfly import homfly_polynomial_string Exception raised: Traceback (most recent call last): File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.libs.homfly.homfly_polynomial_string[0]>", line 1, in <module> from sage.libs.homfly import homfly_polynomial_string ImportError: No module named homfly ********************************************************************** File "src/sage/libs/homfly.pyx", line 91, in sage.libs.homfly.homfly_polynomial_dict Failed example: from sage.libs.homfly import homfly_polynomial_dict Exception raised: Traceback (most recent call last): File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.libs.homfly.homfly_polynomial_dict[0]>", line 1, in <module> from sage.libs.homfly import homfly_polynomial_dict ImportError: No module named homfly **********************************************************************
comment:18 Changed 6 years ago by
 Commit changed from eeda42566c0e90abe38ee9f7669516a647079c68 to b4c923ccd9ec766a035d0fcf33258b655168ec67
Branch pushed to git repo; I updated commit sha1. New commits:
b4c923c  Marked all tests as optional, because they all depend on libhomfly.

comment:19 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 6 years ago by
Looks good to me. It all seems to work ok.
comment:21 Changed 6 years ago by
 Status changed from needs_review to positive_review
I am going to treat that as a positive review.
comment:22 Changed 6 years ago by
 Status changed from positive_review to needs_work
Documentation doesn't build if you don't have libhomfly
comment:23 Changed 6 years ago by
 Commit changed from b4c923ccd9ec766a035d0fcf33258b655168ec67 to 5196caea421e8f6838eb1e2b5eb3b701048c1f76
Branch pushed to git repo; I updated commit sha1. New commits:
5196cae  Attempt to fix the docbuild.

comment:24 Changed 6 years ago by
 Commit changed from 5196caea421e8f6838eb1e2b5eb3b701048c1f76 to b4c923ccd9ec766a035d0fcf33258b655168ec67
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:25 Changed 6 years ago by
No, that was not the right solution.
comment:26 Changed 6 years ago by
 Commit changed from b4c923ccd9ec766a035d0fcf33258b655168ec67 to cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1
Branch pushed to git repo; I updated commit sha1. New commits:
cd786cb  Fixing docbuild issue.

comment:27 Changed 6 years ago by
 Status changed from needs_work to positive_review
Okay, I just copied what was done for the other optional packages.
comment:28 Changed 6 years ago by
 Branch changed from u/tscrim/libhomfly_cython_interface18057 to cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Cython interface to libhomfly
set doctests as optional