-
Notifications
You must be signed in to change notification settings - Fork 2
feat(algorithms, dynamic programming): dynamic programming problems #151
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis change reorganizes palindromic substring problems from the two_pointers directory to the dynamic_programming directory. New implementations for counting palindromic substrings are added, along with comprehensive documentation and corresponding test modules. A minor docstring formatting update is also applied to pascals_triangle. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
algorithms/dynamic_programming/palindromic_substring/longest_palindromic_substring.py (1)
32-32: Critical bug:i + ishould bei + 1.When setting the result bounds for a two-character palindrome, the end index calculation is incorrect. For
phrase[i] == phrase[i + 1], the palindrome spans indices[i, i + 1], not[i, i + i].Example: For input
"aa", wheni=0:
- Current:
result = [0, 0]→ returns"a"- Expected:
result = [0, 1]→ returns"aa"🐛 Proposed fix
if phrase[i] == phrase[i + 1]: dp[i][i + 1] = True - result = [i, i + i] + result = [i, i + 1]
🤖 Fix all issues with AI agents
In @algorithms/dynamic_programming/palindromic_substring/README.md:
- Around line 68-69: Update the sentence that currently reads "lengths greater
than three" to "lengths greater than two" in the README explanation so it
correctly matches the algorithm description (base cases for lengths 1 and 2,
loop starts at length 3); ensure the surrounding text still reads naturally
after this single-word change.
In
@algorithms/dynamic_programming/palindromic_substring/test_longest_palindromic_substring.py:
- Around line 7-15: Add missing edge-case tests to
LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES by including ("", ""), ("a", "a"), and
a length-2 palindrome case such as ("aab", "aa"); also fix the off-by-one bug in
longest_palindromic_substring.py where the center-expansion result is set with
result = [i, i + i] — change it to result = [i, i + 1] so two-character
palindromes are handled correctly (search for the variable name result and the
incorrect assignment to locate the fix).
🧹 Nitpick comments (1)
algorithms/dynamic_programming/palindromic_substring/__init__.py (1)
34-66: Consider documenting the difference between the two implementations.Both functions produce identical results but use slightly different iteration patterns. The second function uses boolean arithmetic (
count += dp[i][j]) which is a nice Pythonic touch.It would be helpful to add a brief note in the docstring or a module-level comment explaining why two implementations exist (e.g., for educational purposes, different iteration order, etc.).
📝 Suggested docstring enhancement
def count_palindromic_substrings_2(s: str) -> int: """ Counts the number of palindromic substrings that can be found in the given string + + This is an alternative implementation that iterates by substring length + rather than by difference between indices. Both approaches have the same + O(n²) time and space complexity. + Args: s(str): string to check for palindromic substrings Returns: int: number of palindromic substrings """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
algorithms/dynamic_programming/palindromic_substring/images/examples/palindromic_substring_example_1.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/examples/palindromic_substring_example_2.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_1.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_10.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_11.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_12.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_13.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_14.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_15.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_16.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_17.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_2.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_3.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_4.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_5.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_6.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_7.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_8.pngis excluded by!**/*.pngalgorithms/dynamic_programming/palindromic_substring/images/solutions/palindromic_substring_solution_9.pngis excluded by!**/*.png
📒 Files selected for processing (10)
DIRECTORY.mdalgorithms/dynamic_programming/palindromic_substring/README.mdalgorithms/dynamic_programming/palindromic_substring/__init__.pyalgorithms/dynamic_programming/palindromic_substring/longest_palindromic_substring.pyalgorithms/dynamic_programming/palindromic_substring/test_longest_palindromic_substring.pyalgorithms/dynamic_programming/palindromic_substring/test_palindromic_substring.pyalgorithms/dynamic_programming/pascals_triangle/README.mdalgorithms/dynamic_programming/pascals_triangle/__init__.pyalgorithms/dynamic_programming/pascals_triangle/test_pascals_triangle.pyalgorithms/two_pointers/palindrome/README.md
💤 Files with no reviewable changes (1)
- algorithms/two_pointers/palindrome/README.md
🧰 Additional context used
🧬 Code graph analysis (1)
algorithms/dynamic_programming/palindromic_substring/test_longest_palindromic_substring.py (1)
algorithms/dynamic_programming/palindromic_substring/longest_palindromic_substring.py (1)
longest_palindromic_substring(4-42)
🪛 LanguageTool
algorithms/dynamic_programming/palindromic_substring/README.md
[style] ~23-~23: Consider a more expressive alternative.
Context: ...results of the earlier computations. To do this, we can create a lookup table, dp,...
(DO_ACHIEVE)
[style] ~43-~43: Using many exclamation marks might seem excessive (in this case: 15 exclamation marks for a text that’s 5567 characters long)
Context: ...s/palindromic_substring_solution_1.png)

🪛 markdownlint-cli2 (0.18.1)
algorithms/dynamic_programming/palindromic_substring/README.md
23-23: Reference links and images should use a label that is defined
Missing link or image reference definition: "j"
(MD052, reference-links-images)
24-24: Reference links and images should use a label that is defined
Missing link or image reference definition: "j"
(MD052, reference-links-images)
33-33: Reference links and images should use a label that is defined
Missing link or image reference definition: "i+1"
(MD052, reference-links-images)
34-34: Reference links and images should use a label that is defined
Missing link or image reference definition: "i+1"
(MD052, reference-links-images)
38-38: Reference links and images should use a label that is defined
Missing link or image reference definition: "1"
(MD052, reference-links-images)
40-40: Reference links and images should use a label that is defined
Missing link or image reference definition: "2"
(MD052, reference-links-images)
DIRECTORY.md
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
🔇 Additional comments (7)
algorithms/dynamic_programming/pascals_triangle/__init__.py (1)
32-32: LGTM!Minor docstring formatting improvement - using
...instead of......for the sequence representation is cleaner and more conventional.algorithms/dynamic_programming/palindromic_substring/test_palindromic_substring.py (1)
1-34: LGTM!Well-structured test module with comprehensive parameterized test cases covering:
- Single character strings (each char is a palindrome)
- All same characters (
"aaa","zzz","wwwwwwwwww")- Mixed strings with various palindromic patterns
- Known palindrome word (
"tattarrattat")The test values are mathematically correct (e.g.,
"wwwwwwwwww"with 10 chars yields 55 palindromic substrings = n*(n+1)/2).DIRECTORY.md (1)
72-77: LGTM!The new directory entries correctly document the added palindromic substring and Pascal's triangle files. The indentation follows the existing pattern used throughout the file.
algorithms/dynamic_programming/palindromic_substring/longest_palindromic_substring.py (1)
1-1: Import path is correct.The
is_palindromefunction is properly exported fromalgorithms.two_pointers.palindrome(defined in__init__.pyat line 1). The import statement is valid and will work as intended.algorithms/dynamic_programming/palindromic_substring/README.md (1)
15-16: All referenced images are present in the repository. The image files at./images/examples/(example_1.png, example_2.png) and./images/solutions/(solution_1.png through solution_17.png) have been included in the PR.algorithms/dynamic_programming/palindromic_substring/test_longest_palindromic_substring.py (1)
18-22: LGTM!The test class structure and parameterized test method are well-implemented. Using
@parameterized.expandis the right approach for data-driven tests.algorithms/dynamic_programming/palindromic_substring/__init__.py (1)
1-31: LGTM!The DP implementation is correct. The algorithm properly:
- Initializes all single-character substrings as palindromes
- Checks adjacent pairs for length-2 palindromes
- Expands to longer lengths using the recurrence relation
dp[i][j] = (s[i] == s[j]) and dp[i+1][j-1]Time complexity is O(n²) and space complexity is O(n²), which is appropriate for this DP approach.
| 3. After these base cases, the algorithm checks all possible substrings of lengths greater than three. However, it only | ||
| compares the first and last characters, and the rest of the substring is checked using the lookup table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor text inconsistency: "three" should be "two".
The solution summary states "lengths greater than three" but should say "lengths greater than two" to be consistent with the algorithm description. Base cases handle lengths 1 and 2, so the loop checks lengths starting from 3 (i.e., greater than 2).
📝 Suggested fix
-3. After these base cases, the algorithm checks all possible substrings of lengths greater than three. However, it only
+3. After these base cases, the algorithm checks all possible substrings of lengths greater than two. However, it only📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. After these base cases, the algorithm checks all possible substrings of lengths greater than three. However, it only | |
| compares the first and last characters, and the rest of the substring is checked using the lookup table. | |
| 3. After these base cases, the algorithm checks all possible substrings of lengths greater than two. However, it only | |
| compares the first and last characters, and the rest of the substring is checked using the lookup table. |
🤖 Prompt for AI Agents
In @algorithms/dynamic_programming/palindromic_substring/README.md around lines
68 - 69, Update the sentence that currently reads "lengths greater than three"
to "lengths greater than two" in the README explanation so it correctly matches
the algorithm description (base cases for lengths 1 and 2, loop starts at length
3); ensure the surrounding text still reads naturally after this single-word
change.
| LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES = [ | ||
| ("mnm", "mnm"), | ||
| ("zzz", "zzz"), | ||
| ("cat", "c"), | ||
| ("lever", "eve"), | ||
| ("xyxxyz", "yxxy"), | ||
| ("wwwwwwwwww", "wwwwwwwwww"), | ||
| ("tattarrattat", "tattarrattat"), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add edge cases for empty string, single character, and length-2 palindromes.
The test suite is missing edge cases that could expose bugs in the implementation:
- Empty string
"" - Single character
"a" - String where the longest palindrome is exactly 2 characters (e.g.,
"aab"→"aa")
The last case is particularly important because the implementation in longest_palindromic_substring.py has a bug on line 29: result = [i, i + i] should be result = [i, i + 1]. This bug would cause incorrect results when the longest palindrome is exactly 2 characters, but none of the current test cases would catch it.
🧪 Suggested additional test cases
LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES = [
+ ("", ""),
+ ("a", "a"),
+ ("aab", "aa"),
+ ("baa", "aa"),
("mnm", "mnm"),
("zzz", "zzz"),
("cat", "c"),
("lever", "eve"),
("xyxxyz", "yxxy"),
("wwwwwwwwww", "wwwwwwwwww"),
("tattarrattat", "tattarrattat"),
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES = [ | |
| ("mnm", "mnm"), | |
| ("zzz", "zzz"), | |
| ("cat", "c"), | |
| ("lever", "eve"), | |
| ("xyxxyz", "yxxy"), | |
| ("wwwwwwwwww", "wwwwwwwwww"), | |
| ("tattarrattat", "tattarrattat"), | |
| ] | |
| LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES = [ | |
| ("", ""), | |
| ("a", "a"), | |
| ("aab", "aa"), | |
| ("baa", "aa"), | |
| ("mnm", "mnm"), | |
| ("zzz", "zzz"), | |
| ("cat", "c"), | |
| ("lever", "eve"), | |
| ("xyxxyz", "yxxy"), | |
| ("wwwwwwwwww", "wwwwwwwwww"), | |
| ("tattarrattat", "tattarrattat"), | |
| ] |
🤖 Prompt for AI Agents
In
@algorithms/dynamic_programming/palindromic_substring/test_longest_palindromic_substring.py
around lines 7 - 15, Add missing edge-case tests to
LONGEST_PALINDROMIC_SUBSTRING_TEST_CASES by including ("", ""), ("a", "a"), and
a length-2 palindrome case such as ("aab", "aa"); also fix the off-by-one bug in
longest_palindromic_substring.py where the center-expansion result is set with
result = [i, i + i] — change it to result = [i, i + 1] so two-character
palindromes are handled correctly (search for the variable name result and the
incorrect assignment to locate the fix).
Describe your change:
Count number of palindromic substrings in a string
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
Documentation
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.