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

Implemented nth Fibonacci number using different methods #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Gyan172004
Copy link

This PR introduces enhancements to the Fibonacci sequence calculation in the "Fibonacci.lua" file. It provides implementations for calculating the nth Fibonacci number using four different approaches with varying time complexities:

  1. O(2^N): fibonacci_recursive(n) - Recursive approach.
  2. O(N): fibonacci_dp(n) - Dynamic programming approach.
  3. O(logN): fibonacci_matrix(n) - Matrix exponentiation approach.
  4. O(1): fibonacci_binet(n) - Binet's formula approach.

Details

  • Added comments and explanations for each function.
  • Included assert tests for all four methods to ensure correctness.
  • Reordered the functions from worst to best time complexity for clarity.

Please review and merge this PR to enhance the Fibonacci calculations in codebase.

Thank you!

Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Please remove the unrelated changes (Sieve of Eratosthenes which we btw already have, albeit with a slightly different interface, package-lock.json).

The structure I envision would be files src/math/fibonacci/recursive.lua, .../fibonacci/binet.lua etc, though packing them up in a table is fine too. Do not use global variables however, use local variables and return "exported" functions or tables appropriately.

The tests effectively all test the same thing (because the functions all fulfill the same spec). Deduplicate them (by extracting their body into a function).

(Note also that they're currently broken: You're not requireing or packing up the functions properly.)

@@ -0,0 +1,80 @@
-- Fibonacci.lua
Copy link
Collaborator

Choose a reason for hiding this comment

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

Useless comment

-- Fibonacci.lua

-- Fibonacci Sequence:
-- The Fibonacci sequence is a series of numbers where each number (after the first two) is the sum of the two preceding ones.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment can stay

-- The Fibonacci sequence is a series of numbers where each number (after the first two) is the sum of the two preceding ones.
-- More information: https://en.wikipedia.org/wiki/Fibonacci_number

-- Author: Gyandeep
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not useful to readers of the code. Your name will be in the commit history if/when this is merged.


-- Author: Gyandeep

-- Implemented nth Fibonacci number using different approaches in O(2^N) , O(N) , O(logN) , O(1) .
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is effectively redundant.



-- Function to calculate Fibonacci numbers using recursion
-- Time Complexity: O(2^n)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inaccurate. Exponential, but not 2^n (it grows with the golden ratio). I'd just use --! exponential time complexity, just for illustrative purposes here.

function fibonacci_binet(n)
local phi = (1 + math.sqrt(5)) / 2
local psi = (1 - math.sqrt(5)) / 2
return math.floor((math.pow(phi, n) - math.pow(psi, n)) / math.sqrt(5))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use math.pow, use ^ instead.

function fibonacci_matrix(n)
if n <= 0 then
return 0
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

else here is unneeded


-- Function to raise a 2x2 matrix to a power n using divide and conquer
-- Time Complexity: O(log(n))
function matrix_power(matrix, n)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is effectively exponentiation by squaring, which we already have and could use with a proper matrix class which provides multiplication. I'm not very keen on duplicating this.


-- Function to multiply two 2x2 matrices
-- Time Complexity: O(1)
function matrix_multiply(a, b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally this should not need to be here if I had finished and pushed my matrix "class" yet :P

Copy link
Collaborator

@appgurueu appgurueu Nov 5, 2023

Choose a reason for hiding this comment

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

if n <= 0 then
return 0
else
local matrix = {1, 1, 1, 0}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This def. warrants a short comment, IMO. The crucial point here is that this matrix maps a vector (a, b) to (b, a + b).

@Panquesito7 Panquesito7 added the enhancement New feature or request label Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants