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

refresh combinat.py #31106

Closed
fchapoton opened this issue Dec 25, 2020 · 15 comments
Closed

refresh combinat.py #31106

fchapoton opened this issue Dec 25, 2020 · 15 comments

Comments

@fchapoton
Copy link
Contributor

  • full flake8
  • some typing annotation
  • using maxima_lib

CC: @tscrim @slel

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: d02b275

Reviewer: David Coudert

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

@fchapoton fchapoton added this to the sage-9.3 milestone Dec 25, 2020
@fchapoton
Copy link
Contributor Author

Commit: ba32878

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/31106

@fchapoton
Copy link
Contributor Author

New commits:

ba32878refresh combinat.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 25, 2020

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

d02b275fix doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 25, 2020

Changed commit from ba32878 to d02b275

@fchapoton
Copy link
Contributor Author

comment:3

green bot, please review

@dcoudert
Copy link
Contributor

comment:4

a small possible correction

-        if (self.first != self.__first_from_iterator and
-                self.next != self.__next_from_iterator):
+        if (self.first != self.__first_from_iterator and
+            self.next != self.__next_from_iterator):

otherwise, LGTM.

@fchapoton
Copy link
Contributor Author

comment:5

ben, je crois que flake8 n'aime pas trop les trucs alignes avec la ligne d'apres..

@dcoudert
Copy link
Contributor

comment:6

Ca explique pourquoi emacs aligne par défaut comme tu l'as fait.

LGTM.

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@slel
Copy link
Member

slel commented Dec 26, 2020

comment:7

En option, même si ce n'est pas le propos principal du ticket,
dans bell_number on pourrait changer R(-1) -> (-R.one()), comme
c'est déjà fait dans bernoulli_polynomial pour ZZ(1) -> ZZ.one().

@slel
Copy link
Member

slel commented Dec 26, 2020

comment:8

Et dans euler_number on pourrait changer

-        sage: 2/(exp(x)+exp(-x))
+        sage: x.cosh().inverse()
         1 - 1/2*x^2 + 5/24*x^4 - 61/720*x^6 + 277/8064*x^8 + O(x^10)

pour gagner quelques microsecondes à chaque doctest

sage: sage: x = PowerSeriesRing(QQ, 'x').gen().O(10)
sage: timeit('2/(exp(x) + exp(-x))')
625 loops, best of 3: 439 μs per loop
sage: timeit('x.cosh().inverse()')
625 loops, best of 3: 250 μs per loop

mais je n'insiste bien sûr pas.

@slel
Copy link
Member

slel commented Dec 26, 2020

comment:9

Dans bernoulli_polynomial on pourrait clarifier ainsi:

-    coeffs = [0] * k + sum(([n.binomial(i) * bernoulli(n - i), 0]
-                            for i in range(k, n + 1, 2)), [])
-    coeffs[-3] = -n / 2
+    coeffs = [0] * (n + 1)
+    coeffs[k::2] = (n.binomial(i) * bernoulli(n - i)
+                    for i in range(k, n + 1, 2))
+    coeffs[-2] = -n / 2

@fchapoton
Copy link
Contributor Author

comment:10

Merci pour les suggestions.
Cependant, ces changements ne me paraissent pas très utiles, donc je préfère en rester là.

@vbraun
Copy link
Member

vbraun commented Dec 27, 2020

Changed branch from u/chapoton/31106 to d02b275

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

4 participants