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

get longest palindrome #704

Closed

Conversation

changweidalaifu6
Copy link
Contributor

Updated code and added comments

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.79%. Comparing base (a247720) to head (11bc144).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   94.55%   94.79%   +0.24%     
==========================================
  Files         296      298       +2     
  Lines       21984    22192     +208     
==========================================
+ Hits        20786    21037     +251     
+ Misses       1198     1155      -43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vil02
Copy link
Member

vil02 commented Apr 16, 2024

Why did you close #703? This PR seems to be exactly the same as #703.

@changweidalaifu6
Copy link
Contributor Author

#703 It's not clear why the other tests didn't pass, I didn't change any of the other code.

@vil02
Copy link
Member

vil02 commented Apr 17, 2024

#703 It's not clear why the other tests didn't pass, I didn't change any of the other code.

It is just some random failure - some tests in this repository are not deterministic. In general - this is not a big deal.

@changweidalaifu6
Copy link
Contributor Author

I thought it was my problem.....

@vil02
Copy link
Member

vil02 commented Apr 17, 2024

In any case, the question about aabb from #703 remains valid.

@changweidalaifu6
Copy link
Contributor Author

aabb theoretically outputs either aa or bb, here it only outputs bb

@changweidalaifu6
Copy link
Contributor Author

I think that should be allowed.

@vil02
Copy link
Member

vil02 commented Apr 17, 2024

The code looks quite good. Please update the doc string by adding some explanation how do you find the longest palindrome, adding a sentence about non-unique solution (I guess, your function returns the one most on the right).

Also, please add a test case with "aabb" (with a suitable comment).

@changweidalaifu6
Copy link
Contributor Author

This compilation error is a machine problem?

}
let s: Vec<char> = s.chars().collect();
let n = s.len();
// dp indicates whether it is a palindrome
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what do you mean here. Maybe try to describe that dp[a][b] is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit redundant here, dp is the string of the dynamic programming process

let n = s.len();
// dp indicates whether it is a palindrome
let mut dp = vec![vec![true; n]; n];
// res record the indexes before and after the palindrome
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about the before part? I am not mistaking, the res[0] is the index of the first character of the resulting palindrome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

res[0] is the index at the beginning of the palindrome,res[1] is the index of the end of the palindrome

let mut res = (0, 0);

// form filling strategy: fill in the form by length
// k denotes the length of the palindrome, the further you traverse, the longer it is
Copy link
Member

Choose a reason for hiding this comment

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

Why not use use palindrome_length instead of k?

// strings of 2 equal characters are palindromes
dp[i][i + k] = s[i] == s[i + 1];
} else {
// a string that is equal on both sides and has an iambic palindrome in the middle is also an iambic palindrome
Copy link
Member

Choose a reason for hiding this comment

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

Please explain what iambic is.

dp[i][i + k] = (s[i] == s[i + k]) && dp[i + 1][i + k - 1];
}

// update palindrome length, record index
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// update palindrome length, record index

Comment on lines 63 to 64
// Theoretically it is possible to return either aa or bb,
// there are multiple palindromes of the same length, here only the rightmost one is returned
Copy link
Member

Choose a reason for hiding this comment

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

I am still thinking about that. In case of "aabb", the longest palindrome does not exist. "aa" and "bb" are palindromes, but they have equal length.

The longest palindrome must be unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point, I don't think it's necessary to get hung up on whether the longest is unique or not,
it's possible for a tournament to have a double winner as well

@vil02
Copy link
Member

vil02 commented Apr 23, 2024

@changweidalaifu6 after rethinking the whole thing, I am not sure anymore if this contribution is suitable for this repository. The problem is very specific (it is a leetcode problem after all) and quite cumbersome to state properly (well, the function should be called something like get_length_of_the_palindrome_with_max_length).

@changweidalaifu6
Copy link
Contributor Author

It's true that it's a bit buttoned up, but it's very tight

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