Opened 3 years ago

Closed 3 years ago

#20075 closed defect (fixed)

rising_factorial and falling_factorial should accept Python integers

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-7.1
Component: basic arithmetic Keywords: factorial
Cc: Merged in:
Authors: Eric Gourgoulhon Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: e77247c (Commits) Commit: e77247ce49586565c975e1a5181b2954afab0532
Dependencies: Stopgaps:

Description

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'

Change History (18)

comment:1 Changed 3 years ago by egourgoulhon

  • Branch set to public/20075
  • Commit set to 11fd5e6099e732148bf385adbbeb99c04cd8a335
  • Keywords factorial added
  • Status changed from new to needs_review

New commits:

​11fd5e6rising_factorial accepts Python integers

comment:2 follow-up: Changed 3 years ago by tscrim

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

comment:3 Changed 3 years ago by git

  • Commit changed from 11fd5e6099e732148bf385adbbeb99c04cd8a335 to 23fb2b1857cdba07e431c0e7521490c12e857ff5

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

​23fb2b1rising_factorial and falling_factorial accept Python integers (int, long)

comment:4 in reply to: ↑ 2 Changed 3 years ago by egourgoulhon

  • Summary changed from rising_factorial should accept Python integers to rising_factorial and falling_factorial should accept Python integers

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.

comment:5 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to needs_work

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.

comment:6 follow-ups: Changed 3 years ago by vdelecroix

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

comment:7 in reply to: ↑ 6 Changed 3 years ago by tscrim

Replying to vdelecroix:

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.

comment:8 in reply to: ↑ 6 Changed 3 years ago by egourgoulhon

Replying to vdelecroix:

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

comment:9 follow-up: Changed 3 years ago by vdelecroix

yep. I read too fast. Sorry for that.

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

comment:10 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by egourgoulhon

Replying to vdelecroix:

yep. I read too fast. Sorry for that.

But 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?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by 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).

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by vdelecroix

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

comment:13 in reply to: ↑ 12 Changed 3 years ago by egourgoulhon

Replying to vdelecroix:

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

comment:14 Changed 3 years ago by git

  • Commit changed from 23fb2b1857cdba07e431c0e7521490c12e857ff5 to e77247ce49586565c975e1a5181b2954afab0532

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

​e77247cUse py_scalar_to_element in rising_factorial and falling_factorial

comment:15 Changed 3 years ago by egourgoulhon

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 3 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Sorry for the long delay...

comment:17 in reply to: ↑ 16 Changed 3 years ago by egourgoulhon

Replying to vdelecroix:

Sorry for the long delay...

No problem. Thank you Vincent !

comment:18 Changed 3 years ago by vbraun

  • Branch changed from public/20075 to e77247ce49586565c975e1a5181b2954afab0532
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.