rising_factorial and falling_factorial should accept Python integers

[egourgoulhon]

See the bug reported at
http://ask.sagemath.org/question/32565/error-in-rising_factorial/

sage: [rising_factorial(n,n) for n in range(6)]
AttributeError: 'int' object has no attribute 'parent'

Component: basic arithmetic

Keywords: factorial

Author: Eric Gourgoulhon

Branch/Commit: e77247c

Reviewer: Vincent Delecroix

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

[egourgoulhon]

Changed keywords from none to factorial

[egourgoulhon]

Commit: 11fd5e6

[egourgoulhon]

Branch: public/20075

[egourgoulhon]

New commits:

11fd5e6

rising_factorial accepts Python integers

[tscrim]

comment:2

Since there is a belief that the input could be a long, I think you should also check against that as well.

[sagetrac-git]

Changed commit from 11fd5e6 to 23fb2b1

[sagetrac-git]

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

23fb2b1

rising_factorial and falling_factorial accept Python integers (int, long)

[egourgoulhon]

comment:4

Replying to @tscrim:

Since there is a belief that the input could be a long, I think you should also check against that as well.

Done in the above commit. Btw, I've also modified falling_factorial for consistency.

[videlec]

comment:5

You would better use sage.structure.coerce.py_scalar_to_element. For example you forgot about numpy integers. You can simply add the line x = py_scalar_to_element(x) at the begining.

[videlec]

comment:6

And since int,long are changed to Integer there is no need to check for these types in the second pass.

[tscrim]

comment:7

Replying to @videlec:

And since int,long are changed to Integer there is no need to check for these types in the second pass.

The first check is x and the second is a.

[egourgoulhon]

comment:8

Replying to @videlec:

And since int,long are changed to Integer there is no need to check for these types in the second pass.

What do you mean? In the code of these functions, there is no second pass for x (the other checks regard a, not x).

[videlec]

comment:9

yep. I read too fast. Sorry for that.

But [comment:5] can be applied to both x and a (and hence the code will also work with numpy integers).

[egourgoulhon]

comment:10

Replying to @videlec:

yep. I read too fast. Sorry for that.

But [comment:5] can be applied to both x and a (and hence the code will also work with numpy integers).

In the current code, there is no need to force a to be a Sage element, for only x.parent() is invoked. So I would apply py_scalar_to_element only to x. Do you agree?

[egourgoulhon]

comment:11

Replying to @egourgoulhon:

In the current code, there is no need to force a to be a Sage element, for only x.parent() is invoked. So I would apply py_scalar_to_element only to x. Do you agree?

PS: in particular, the current code already works with a being a numpy integer (I've just checked it).

[videlec]

comment:12

Replying to @egourgoulhon:

Replying to @egourgoulhon:

In the current code, there is no need to force a to be a Sage element, for only x.parent() is invoked. So I would apply py_scalar_to_element only to x. Do you agree?

PS: in particular, the current code already works with a being a numpy integer (I've just checked it).

Are you sure? What kind of numpy integers did you try?

sage: import numpy
sage: a = numpy.int8(10)
sage: b = numpy.int(5)
sage: isinstance(a, int)
False
sage: isinstance(b, int)
True

[egourgoulhon]

comment:13

Replying to @videlec:

Are you sure? What kind of numpy integers did you try?

sage: import numpy
sage: a = numpy.int8(10)
sage: b = numpy.int(5)
sage: isinstance(a, int)
False
sage: isinstance(b, int)
True

Both work:

sage: import numpy
sage: a = numpy.int8(4)
sage: rising_factorial(3, a)
360
sage: a = numpy.int(4)
sage: rising_factorial(3, a)
360

[sagetrac-git]

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

e77247c

Use py_scalar_to_element in rising_factorial and falling_factorial

[sagetrac-git]

Changed commit from 23fb2b1 to e77247c

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:16

Sorry for the long delay...

[egourgoulhon]

comment:17

Replying to @videlec:

Sorry for the long delay...

No problem. Thank you Vincent !

[vbraun]

Changed branch from public/20075 to e77247c