Opened 10 years ago

Closed 5 years ago

#9248 closed defect (fixed)

docstring for factorial doesn't say that it accepts non-integer, non-symbolic input

Reported by: ddrake Owned by: ddrake
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: documentation Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

At comment:2:ticket:9240, we see someone get confused about the docstring for factorial, which claims it takes as input an integer or symbolic expression. However, it takes non-integer, non-SR inputs:

sage: x = 1.5; parent(x)
Real Field with 53 bits of precision
sage: factorial(x)
1.32934038817914
sage: x = 3/2; parent(x)
Rational Field
sage: factorial(x)      
3/4*sqrt(pi)
sage: x = CC(1+I); parent(x)
Complex Field with 53 bits of precision
sage: factorial(x)
0.652965496420167 + 0.343065839816545*I

I understand that there is coercion going on, but we should specify that the function takes pretty much any complex number (except of course negative integers) and evaluates (something akin to) gamma(1+x).

However, it doesn't exactly do gamma(1+x):

sage: x = I; parent(x)  
Symbolic Ring
sage: factorial(x)    
0.498015668118356 - 0.154949828301811*I
sage: gamma(x+1)      
gamma(I + 1)
sage: parent(factorial(x))  
Symbolic Ring
sage: gamma(x+1).n() 
0.498015668118356 - 0.154949828301811*I
sage: parent(gamma(x+1).n())
Complex Field with 53 bits of precision

The factorial function clearly is not simply calling gamma(x+1) when x is not an integer.

Change History (14)

comment:1 follow-up: Changed 10 years ago by ddrake

  • Owner changed from mvngu to ddrake

Also, factorial doesn't seem to accept the algorithm keyword anymore, even though the docstring says it does! I see that factorial() is just a wrapper around GiNaC's factorial; how does GiNaC compute factorials?

comment:2 follow-up: Changed 9 years ago by kcrisman

Notice also that factorial does not accept all sorts of input, which leads to problems in this thread.

comment:3 in reply to: ↑ 2 Changed 9 years ago by kcrisman

Replying to kcrisman:

Notice also that factorial does not accept all sorts of input, which leads to problems in this thread.

Which for some reason I still posted even after realizing this is just #9240. Sorry for the noise.

comment:4 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 in reply to: ↑ 1 Changed 5 years ago by rws

Replying to ddrake:

Also, factorial doesn't seem to accept the algorithm keyword anymore, even though the docstring says it does!

Wrong doc read. The problem is fixed in #17489.

comment:9 Changed 5 years ago by rws

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

It appears the originally described issue was fixed in #12286 (misleading title).

comment:10 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

please change the status of a ticket to 'positive review' when you flag it as wontfix.

comment:11 follow-up: Changed 5 years ago by tscrim

The same person should not both set it to wontfix and give it a positive review as it skips the review process.

comment:12 Changed 5 years ago by jdemeyer

On the other hand, closing a ticket without review is much less bad than merging a branch without review. And essentially nobody reviews sage-duplicate/invalid/wontfix tickets.

comment:13 in reply to: ↑ 11 Changed 5 years ago by kcrisman

The same person should not both set it to wontfix and give it a positive review as it skips the review process.

But the invalid and duplicate I think this is okay for. I often give a positive review to dups or obviously now-functioning tickets.

And essentially nobody reviews sage-duplicate/invalid/wontfix tickets.

Yeah, probably also true.

I do have to say that perhaps we should have a separate wontfix that would require two reviewers, maybe? I have occasionally set some notebook tickets to wontfix but that is a fairly unusual situation.

comment:14 Changed 5 years ago by vbraun

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