Opened 5 years ago

Closed 3 years ago

#18057 closed enhancement (fixed)

Cython interface to libhomfly

Reported by: mmarco Owned by:
Priority: major Milestone: sage-7.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) Commit: cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1
Dependencies: Stopgaps:

Description

This implements a cython interface to the libhomfly library in #18047

Change History (28)

comment:1 Changed 5 years ago by mmarco

  • Branch set to u/mmarco/libhomfly_interface

comment:2 Changed 4 years ago by jdemeyer

  • Commit set to d3e96fc26750c34166cff23b64a706ebc6260b63
  • Component changed from interfaces to interfaces: optional

New commits:

86b9a43Cython interface to libhomfly
d3e96fcset doctests as optional

comment:3 Changed 4 years ago by mmarco

  • Cc fugelde tscrim added

comment:4 Changed 3 years ago by git

  • Commit changed from d3e96fc26750c34166cff23b64a706ebc6260b63 to 69ec550c0ff5ffc526b0ceb06bc899e166221f2f

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

fa1db68Cython interface to libhomfly
0f880bcset doctests as optional
69ec550Merge branch 'u/mmarco/libhomfly_interface' of git://trac.sagemath.org/sage into t/18057/libhomfly_interface

comment:5 Changed 3 years ago by mmarco

  • Keywords days74 added
  • Status changed from new to needs_review

Rebased to 7.3 beta2


New commits:

fa1db68Cython interface to libhomfly
0f880bcset doctests as optional
69ec550Merge branch 'u/mmarco/libhomfly_interface' of git://trac.sagemath.org/sage into t/18057/libhomfly_interface

comment:6 Changed 3 years ago by git

  • Commit changed from 69ec550c0ff5ffc526b0ceb06bc899e166221f2f to 2019b9d67621ece7f830a8f20e6d568af6dc8713

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

2019b9dMoved dependency to cysignals

comment:7 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Delete this commented-out code:

#clib homfly
#clib gc

comment:8 Changed 3 years ago by jdemeyer

You should use OptionalExtension, see other examples in module_list.py.

comment:9 Changed 3 years ago by git

  • Commit changed from 2019b9d67621ece7f830a8f20e6d568af6dc8713 to 53ad4bffbcfe4d5ee5f13b7a5383c15d5b9e858b

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

53ad4bfAdapted to the new interface with functions for strings and dict

comment:10 Changed 3 years ago by jdemeyer

  • Dependencies #18047 deleted

This technically does not depend on #18047.

comment:11 Changed 3 years ago by tscrim

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

I think you are right, we should free it.

comment:13 follow-up: Changed 3 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 3 years ago by tscrim

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 free-ing ourselves.

comment:15 Changed 3 years ago by tscrim

  • Branch changed from u/mmarco/libhomfly_interface to u/tscrim/libhomfly_cython_interface-18057
  • Commit changed from 53ad4bffbcfe4d5ee5f13b7a5383c15d5b9e858b to eeda42566c0e90abe38ee9f7669516a647079c68
  • Milestone changed from sage-6.6 to sage-7.3
  • Reviewers set to Jeroen Demeyer, Travis Scrimshaw

I've made this OptionalExtension and removed the commented out code.


New commits:

eeda425Removing commented out code and making OptionalExtension.

comment:16 Changed 3 years ago by tscrim

  • Status changed from needs_work to needs_review

comment:17 Changed 3 years ago by jdemeyer

  • 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/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/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/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/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 3 years ago by git

  • Commit changed from eeda42566c0e90abe38ee9f7669516a647079c68 to b4c923ccd9ec766a035d0fcf33258b655168ec67

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

b4c923cMarked all tests as optional, because they all depend on libhomfly.

comment:19 Changed 3 years ago by tscrim

  • Status changed from needs_work to needs_review

comment:20 Changed 3 years ago by mmarco

Looks good to me. It all seems to work ok.

comment:21 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

I am going to treat that as a positive review.

comment:22 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Documentation doesn't build if you don't have libhomfly

comment:23 Changed 3 years ago by git

  • Commit changed from b4c923ccd9ec766a035d0fcf33258b655168ec67 to 5196caea421e8f6838eb1e2b5eb3b701048c1f76

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

5196caeAttempt to fix the docbuild.

comment:24 Changed 3 years ago by git

  • 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 3 years ago by tscrim

No, that was not the right solution.

comment:26 Changed 3 years ago by git

  • Commit changed from b4c923ccd9ec766a035d0fcf33258b655168ec67 to cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1

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

cd786cbFixing docbuild issue.

comment:27 Changed 3 years ago by tscrim

  • Status changed from needs_work to positive_review

Okay, I just copied what was done for the other optional packages.

comment:28 Changed 3 years ago by vbraun

  • Branch changed from u/tscrim/libhomfly_cython_interface-18057 to cd786cbf75fcd49339c0ed60f3ff3373ff0ee2f1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.