some details in sloane_functions

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.39k stars 473 forks source link

some details in sloane_functions #32397

Closed fchapoton closed 3 years ago

fchapoton commented 3 years ago

CC: @tscrim @slel

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: 8ffa650

Reviewer: Travis Scrimshaw

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

fchapoton commented 3 years ago

Commit: 51d7411

fchapoton commented 3 years ago

New commits:

51d7411 a few details in sloane_functions

fchapoton commented 3 years ago

Branch: u/chapoton/32397

tscrim commented 3 years ago

comment:3

I did some more cleanup of the documentation and some PEP8 stuff. There are a lot of changes, but it is mostly from doing find-and-replace. If my changes are good, positive review.

New commits:

8ffa650 More cleanup and changes of sloane_functions.

tscrim commented 3 years ago

Changed branch from u/chapoton/32397 to u/tscrim/32397

tscrim commented 3 years ago

Reviewer: Travis Scrimshaw

tscrim commented 3 years ago

Changed commit from 51d7411 to 8ffa650

fchapoton commented 3 years ago

comment:4

ok, thanks. Let it be.

slel commented 3 years ago

comment:5

Can you comment on this and similar changes:

-        return Integer(n)
+        return ZZ(n)

I was under the vague impression Integer(n) should be preferred to ZZ(n).

Maybe because it's more direct? or slightly faster? Doesn't ZZ(n) call Integer(n) in the end?

fchapoton commented 3 years ago

comment:6

Travis removed an alias in this file that said Integer = ZZ ! So the behaviour is the exact same behaviour as it was before.

tscrim commented 3 years ago

comment:7

Replying to @slel:

I was under the vague impression Integer(n) should be preferred to ZZ(n).

Maybe because it's more direct? or slightly faster? Doesn't ZZ(n) call Integer(n) in the end?

I am never really sure which one to use. From a quick look, by calling Integer, you avoid a trip through Sage's coercion check as ZZ._element_constructor_ is Integer. So Integer(n) is faster, but maybe less robust? Anyways, as Frédéric mentioned, this was just getting rid of an alias. Maybe I should have gotten rid of the ZZ(n) calls and imported Integer. Well, either way, I doubt this will be a bottleneck in any code and have an impact.

slel commented 3 years ago

comment:8

Importing ZZ is needed here in any case for ZZ.zero() and ZZ.one().

I also doubt a few tens of nanoseconds will impact OEIS-related code.

All in all I am fine with either solution, I was just curious.

vbraun commented 3 years ago

Changed branch from u/tscrim/32397 to 8ffa650