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

Least Common Multiple #99

Merged
merged 3 commits into from
Aug 4, 2015
Merged

Least Common Multiple #99

merged 3 commits into from
Aug 4, 2015

Conversation

guliash
Copy link
Contributor

@guliash guliash commented Aug 3, 2015

LCM algorithms based on the implemented GCD algorithms.

@guliash
Copy link
Contributor Author

guliash commented Aug 4, 2015

There are some corner cases which I haven't handled. For example, lcm(0,0) = 0 by the wiki definition. Will fix that a little bit later.

@felipernb
Copy link
Owner

LGTM

When you have the corner cases implemented (with tests) I'm happy to merge.

Thanks!

@juanplopes
Copy link
Contributor

Just one little nit picking: I'd rather go with the a / gcd(a, b) * b version, because even in Javascript all numbers being 64-bit floats, you can have numerical stability issues. Consider computing lcm(1.1e20, 1.1e20) for example, 1.1e20*1.1e20/1.1e20 !== 1.1e20/1.1e20*1.1e20 just because of precision issues.

This version is also better for people who are learning this algorithm for the first time, so when they implement it in their language of choice (that may have platform integers), it won't have this overflow bug that cuts the precision of the algorithm in half.

@guliash
Copy link
Contributor Author

guliash commented Aug 4, 2015

Improved the code a little. Now it handles case lcm(a, 0) and always returns a positive result even if the inputs are negative. And as @juanplopes suggested changed the order of operations.

felipernb added a commit that referenced this pull request Aug 4, 2015
Least Common Multiple
@felipernb felipernb merged commit ff31975 into felipernb:master Aug 4, 2015
@guliash guliash deleted the lcm branch August 6, 2015 09:27
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

3 participants