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

allow to use flint for Stirling numbers #33733

Closed
fchapoton opened this issue Apr 19, 2022 · 22 comments
Closed

allow to use flint for Stirling numbers #33733

fchapoton opened this issue Apr 19, 2022 · 22 comments

Comments

@fchapoton
Copy link
Contributor

of both kind

CC: @tscrim @slel @kliem @kwankyu

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: bbd7807

Reviewer: Marc Mezzarobba, Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-9.6 milestone Apr 19, 2022
@fchapoton
Copy link
Contributor Author

Commit: adf1244

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/33733

@fchapoton
Copy link
Contributor Author

New commits:

adf1244allow to use flint for Stirling numbers of both kind

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2022

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

722c2dbfix indentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2022

Changed commit from adf1244 to 722c2db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2022

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

384ac6eremove duplicate TEST

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2022

Changed commit from 722c2db to 384ac6e

@fchapoton
Copy link
Contributor Author

comment:4

green bot, so please review

@fchapoton
Copy link
Contributor Author

comment:5

Maybe one should not remove the maxima algorithm, which is not using the pexpect interface in fact ?

@mezzarobba
Copy link
Member

comment:6

I agree that it would be better not to remove the algorithm="maxima" option without deprecating it first.

As far as I understand, the native Sage implementation of the Stirling numbers of the second kind and the Sage implementation were both written by Fredrik. Is the native Sage version better in some way? Do we need to keep it at all?

Would it make sense to change the default implementation for some of the functions?

A minor style point: why do you write

def stirling_number1(n, k, algorithm=None) -> Integer:
    ...
    if algorithm is None:
        algorithm='gap'

rather than

def stirling_number1(n, k, algorithm="gap") -> Integer:
    ...

?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2022

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

42d5b69Merge branch 'u/chapoton/33733' in 9.6.rc1
bbd7807re-add maxima algo for Stirling

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2022

Changed commit from 384ac6e to bbd7807

@fchapoton
Copy link
Contributor Author

comment:8

voila, j'ai remis l'algorithme "maxima" en place

@fchapoton
Copy link
Contributor Author

comment:9

et le patchbot est tout vert

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:11

please review

@tscrim
Copy link
Collaborator

tscrim commented Apr 29, 2022

comment:12

What about Marc's other questions in comment:6?

@fchapoton
Copy link
Contributor Author

comment:13

As far as I understand, the native Sage implementation of the Stirling numbers of the second kind and the Sage implementation were both written by Fredrik. Is the native Sage version better in some way? Do we need to keep it at all?

Would it make sense to change the default implementation for some of the functions?

I have no idea, and do not plan to investigate

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 May 3, 2022
@fchapoton
Copy link
Contributor Author

comment:15

may we please move forward here ?

@tscrim
Copy link
Collaborator

tscrim commented May 20, 2022

Reviewer: Marc Mezzarobba, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented May 20, 2022

comment:16

Sorry, I lost track of this. Thank you. LGTM.

@vbraun
Copy link
Member

vbraun commented May 22, 2022

Changed branch from u/chapoton/33733 to bbd7807

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