Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#23498 closed enhancement (fixed)

Ship Sebastian Pancratz's deformation code

Reported by: jpflori Owned by:
Priority: major Milestone: sage-8.1
Component: packages: optional Keywords: sd87
Cc: edgarcosta, vbraun Merged in:
Authors: Jean-Pierre Flori Reviewers: Edgar Costa
Report Upstream: N/A Work issues:
Branch: 76009ed (Commits, GitHub, GitLab) Commit:
Dependencies: #23466 Stopgaps:

Status badges

Description (last modified by jpflori)

Tarball at: ​http://perso.telecom-paristech.fr/~flori/sage/deformation-d05941b.tar.bz2

Original code from Sebastian at: ​https://github.com/SPancratz/deformation/ modified by JP: ​https://github.com/jpflori/deformation/

Change History (17)

comment:1 Changed 4 years ago by jpflori

  • Keywords sd87 added

comment:2 Changed 4 years ago by git

  • Commit changed from 10f88ff789f78b43d5beafddcdd78df88e516cc4 to 2baec50c82a5223902238747f7f56abf79936d6c

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

​2baec50Use git commit sha1 as version for the moment.

comment:3 Changed 4 years ago by jpflori

  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 Changed 4 years ago by git

  • Commit changed from 2baec50c82a5223902238747f7f56abf79936d6c to af22d831c21ef78b88b2f27543c790ccef1eeb77

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

​af22d83Update deformation to include license.

comment:5 Changed 4 years ago by vdelecroix

In the flint declaration file you are giving access to the precise structure of the C struct, namely 

cdef extern from "flint/fmpz_poly.h":
    ctypedef struct fmpz_poly_struct:
        fmpz* coeffs
        long alloc
        long length

This not part of the public API of flint and would better be avoided in public Cython headers. Moreover, flint provides macros for accessing them. Please change to 

cdef extern from "flint/fmpz_poly.h":
    ctypedef struct fmpz_poly_struct:
        pass

comment:6 Changed 4 years ago by vdelecroix

BTW, what these flint declarations have to do with the ticket!?

comment:7 Changed 4 years ago by jpflori

  • Dependencies set to #23466

These changes are from #23466. I added flint types there and cleaned up the previous pxd files a little bit. I agree we could live without the internal data structure of flint but I did not check we never actually use them in sage.

comment:8 Changed 4 years ago by jpflori

(Note that I did not expand the fmpz_poly declaration, I just moved it so that it appears at a more natural place in types.pxd.)

comment:9 Changed 4 years ago by git

  • Commit changed from af22d831c21ef78b88b2f27543c790ccef1eeb77 to 76009eda90d5eb368725d6791825d9582efa3129

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

​76009edTypo.

comment:10 Changed 4 years ago by edgarcosta

  • Status changed from needs_review to positive_review

comment:11 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name missing

comment:12 Changed 4 years ago by jpflori

  • Reviewers set to Edgar Costa
  • Status changed from needs_work to positive_review

comment:13 Changed 4 years ago by vbraun

  • Branch changed from u/jpflori/deformation to 76009eda90d5eb368725d6791825d9582efa3129
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 Changed 4 years ago by fbissey

  • Commit 76009eda90d5eb368725d6791825d9582efa3129 deleted

Probably need a follow up because #23179 was merged at the same time. spkg-check and spkg-install shouldn't start with #!/usr/bin/env bash or equivalent anymore and they shouldn't be executable (644 instead of the current 755) either.

comment:15 Changed 4 years ago by jpflori

  • Cc vbraun added
  • Description modified (diff)

And it also seems I forgot to update the ticket description with the latest tarball version. The correct one is at:

@volker: can you upload the correct tarball on the mirrors? sorry about the mess...

comment:16 Changed 4 years ago by jpflori

Follow up for spkg-install at #23604

comment:17 Changed 4 years ago by vbraun

ok done

Note: See TracTickets for help on using tickets.