Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added factorial function to int and float #3504

Closed
wants to merge 1 commit into from

Conversation

dylandy
Copy link

@dylandy dylandy commented Nov 5, 2016

This method can behave better than Range#product method which can get value up to 170, the same behaviour as R lang does.

Also I added float number factorial calculation using gamma function.

@david50407
Copy link
Contributor

Actually, you can melt these commits together before you create pull request.
and why re-format some files which are irrelevant to this change?

@@ -96,6 +96,10 @@ struct Float
def self.from_io(io : IO, format : IO::ByteFormat)
format.decode(self, io)
end

def factorial
self*Math.gamma(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no space in between.

@dylandy dylandy force-pushed the factorial branch 2 times, most recently from 0b1b1bd to de36fe3 Compare November 6, 2016 02:27
@dylandy
Copy link
Author

dylandy commented Nov 6, 2016

@david50407 @Sija I just rebased those commits and recovered the other files that I accidentally formatted.

@@ -553,6 +553,10 @@ struct Int
self
end
end

def factorial
self == 0 ? 1.0 : (1..self).to_a.map { |x| x.to_f64 }.reduce { |x, y| x*y }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and spaces here { |x, y| x * y }

@dylandy
Copy link
Author

dylandy commented Nov 6, 2016

fixed the format issue in int.cr.

@asterite
Copy link
Member

asterite commented Nov 6, 2016

Thank you!

But... I never used factorial in my life, and never saw it used except for code samples and blog posts. What's a real use case for factorial other than that? Also, factorial gets big pretty quickly, which means that using limited integers like Int32 isn't very good.

@oprypin
Copy link
Member

oprypin commented Nov 6, 2016

If someone needs factorial, they can decide for themselves what number type they want to work with and (very easily) implement factorial accordingly. Int is not always a good fit because it overflows quickly, and Float just doesn't feel right.

I think that this should not be a method on the number data types. Might be OK for the Math module, but still...

There's probably a good reason that Ruby doesn't have factorial.

@@ -553,6 +553,10 @@ struct Int
self
end
end

def factorial
self == 0 ? 1.0 : (1..self).to_a.map { |x| x.to_f64 }.reduce { |x, y| x * y }
Copy link
Contributor

@ysbaddaden ysbaddaden Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of code, we can avoid the arrays by using an iterator, and pass 1.0 to reduce to avoid casting all values to Float64:

self == 0 ? 1.0 : 1.upto(self).reduce(1.0) { |a, e| a * e }

Still, this is the basic algorithm, exact but slow, and limited to 170!.

@ysbaddaden
Copy link
Contributor

I follow up on what @asterite said. I'm not sure this is any common to be included in the standard library. It may prove to be more useful in an algebra shard, along with many more mathematical algorithms.

@dylandy
Copy link
Author

dylandy commented Nov 7, 2016

Thanks for reply, I'm working on some machine learning projects which needs a lots of performance and I was really upset about how slow Python and R did so I decided to move on to Crystal. But some of the basic and most use functions have missed here, so I just give it a try though.

improved factorial function with avoid changing it into array

reformating int
@mverzilli
Copy link

Closing. There's consensus this doesn't need to be in the stdlib.

@mverzilli mverzilli closed this Jun 17, 2017
@straight-shoota straight-shoota mentioned this pull request Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants