Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ship Sebastian Pancratz's deformation code #23498

Closed
jpflori opened this issue Jul 20, 2017 · 22 comments
Closed

Ship Sebastian Pancratz's deformation code #23498

jpflori opened this issue Jul 20, 2017 · 22 comments

Comments

@jpflori
Copy link

jpflori commented Jul 20, 2017

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/

Depends on #23466

CC: @edgarcosta @vbraun

Component: packages: optional

Keywords: sd87

Author: Jean-Pierre Flori

Branch: 76009ed

Reviewer: Edgar Costa

Issue created by migration from https://trac.sagemath.org/ticket/23498

@jpflori jpflori added this to the sage-8.1 milestone Jul 20, 2017
@jpflori
Copy link
Author

jpflori commented Jul 20, 2017

Changed keywords from none to sd87

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

Changed commit from 10f88ff to 2baec50

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

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

2baec50Use git commit sha1 as version for the moment.

@jpflori

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

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

af22d83Update deformation to include license.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 20, 2017

Changed commit from 2baec50 to af22d83

@videlec
Copy link
Contributor

videlec commented Jul 21, 2017

comment:5

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

@videlec
Copy link
Contributor

videlec commented Jul 21, 2017

comment:6

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

@jpflori
Copy link
Author

jpflori commented Jul 21, 2017

comment:7

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.

@jpflori
Copy link
Author

jpflori commented Jul 21, 2017

Dependencies: #23466

@jpflori
Copy link
Author

jpflori commented Jul 21, 2017

comment:8

(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.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2017

Changed commit from af22d83 to 76009ed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2017

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

76009edTypo.

@vbraun
Copy link
Member

vbraun commented Jul 30, 2017

comment:11

Reviewer name missing

@jpflori
Copy link
Author

jpflori commented Jul 30, 2017

Reviewer: Edgar Costa

@vbraun
Copy link
Member

vbraun commented Aug 1, 2017

Changed branch from u/jpflori/deformation to 76009ed

@kiwifb
Copy link
Member

kiwifb commented Aug 1, 2017

Changed commit from 76009ed to none

@kiwifb
Copy link
Member

kiwifb commented Aug 1, 2017

comment:14

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.

@jpflori

This comment has been minimized.

@jpflori
Copy link
Author

jpflori commented Aug 8, 2017

comment:15

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...

@jpflori
Copy link
Author

jpflori commented Aug 9, 2017

comment:16

Follow up for spkg-install at #23604

@vbraun
Copy link
Member

vbraun commented Aug 9, 2017

comment:17

ok done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants