#31106 closed enhancement (fixed)

refresh combinat.py

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-9.3
Component: combinatorics Keywords:
Cc: tscrim, slelievre Merged in:
Authors: Frédéric Chapoton Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: d02b275 (Commits, GitHub, GitLab) Commit: d02b2750a1ca3406fd632662e2a3ae71c6334375
Dependencies: Stopgaps:

Status badges

Description

  • full flake8
  • some typing annotation
  • using maxima_lib

Change History (11)

comment:1 Changed 12 months ago by chapoton

  • Branch set to u/chapoton/31106
  • Commit set to ba32878b2bdd8604c921d3eccbfe73886f9b809c
  • Status changed from new to needs_review

New commits:

ba32878refresh combinat.py

comment:2 Changed 12 months ago by git

  • Commit changed from ba32878b2bdd8604c921d3eccbfe73886f9b809c to d02b2750a1ca3406fd632662e2a3ae71c6334375

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

d02b275fix doctest

comment:3 Changed 12 months ago by chapoton

  • Cc tscrim slelievre added

green bot, please review

comment:4 Changed 12 months ago by dcoudert

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.

comment:5 Changed 12 months ago by chapoton

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

comment:6 Changed 12 months ago by dcoudert

  • Reviewers set to David Coudert
  • Status changed from needs_review to positive_review

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

LGTM.

comment:7 Changed 12 months ago by slelievre

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

Last edited 12 months ago by slelievre (previous) (diff)

comment:8 Changed 12 months ago by slelievre

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.

Last edited 12 months ago by slelievre (previous) (diff)

comment:9 Changed 12 months ago by slelievre

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

comment:10 Changed 12 months ago by chapoton

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

comment:11 Changed 11 months ago by vbraun

  • Branch changed from u/chapoton/31106 to d02b2750a1ca3406fd632662e2a3ae71c6334375
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.