Opened 6 years ago
Closed 6 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, GitHub, GitLab) | 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 6 years ago by
- Branch set to public/20075
- Commit set to 11fd5e6099e732148bf385adbbeb99c04cd8a335
- Keywords factorial added
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 6 years ago by
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 6 years ago by
- Commit changed from 11fd5e6099e732148bf385adbbeb99c04cd8a335 to 23fb2b1857cdba07e431c0e7521490c12e857ff5
Branch pushed to git repo; I updated commit sha1. New commits:
23fb2b1 | rising_factorial and falling_factorial accept Python integers (int, long)
|
comment:4 in reply to: ↑ 2 Changed 6 years ago by
- 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 6 years ago by
- 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: ↓ 7 ↓ 8 Changed 6 years ago by
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 6 years ago by
Replying to vdelecroix:
And since
int,long
are changed toInteger
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 6 years ago by
Replying to vdelecroix:
And since
int,long
are changed toInteger
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: ↓ 10 Changed 6 years ago by
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: ↓ 11 Changed 6 years ago by
Replying to vdelecroix:
yep. I read too fast. Sorry for that.
But 5 can be applied to both
x
anda
(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: ↓ 12 Changed 6 years ago by
Replying to egourgoulhon:
In the current code, there is no need to force
a
to be a Sage element, for onlyx.parent()
is invoked. So I would applypy_scalar_to_element
only tox
. 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: ↓ 13 Changed 6 years ago by
Replying to egourgoulhon:
Replying to egourgoulhon:
In the current code, there is no need to force
a
to be a Sage element, for onlyx.parent()
is invoked. So I would applypy_scalar_to_element
only tox
. 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 6 years ago by
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 6 years ago by
- Commit changed from 23fb2b1857cdba07e431c0e7521490c12e857ff5 to e77247ce49586565c975e1a5181b2954afab0532
Branch pushed to git repo; I updated commit sha1. New commits:
e77247c | Use py_scalar_to_element in rising_factorial and falling_factorial
|
comment:15 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:16 follow-up: ↓ 17 Changed 6 years ago by
- 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 6 years ago by
comment:18 Changed 6 years ago by
- Branch changed from public/20075 to e77247ce49586565c975e1a5181b2954afab0532
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
rising_factorial accepts Python integers