Skip to content

Changed knuth_morris_pratt to be consistent with str.find() #9079

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

Closed
wants to merge 9 commits into from

Conversation

Akashram28
Copy link

@Akashram28 Akashram28 commented Sep 23, 2023

Describe your change:

function kmp() now returns the first occurrence of the pattern. If the pattern does not exist in the given text, it returns -1

See #9077

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes Returning index instead of boolean when knuth_morris_pratt matches. #9077".

@rohan472000
Copy link
Contributor

rohan472000 commented Sep 23, 2023

Change bool return type to int for kmp function.

@rohan472000
Copy link
Contributor

In your PR description, you should tick whatever suits your PR but don't delete other checkboxes, also tick that checkbox for bug fixing or doc change which u deleted.

@rohan472000
Copy link
Contributor

@cclauss , kindly review it.

@amirsoroush
Copy link
Contributor

Thanks. "doctests" are more desirable than print statements. While we're at it, let's write some tests instead.

@amirsoroush
Copy link
Contributor

amirsoroush commented Sep 23, 2023

Just to note, you can use some keywords in your commit message to link a pull request to an issue, that way if your PR is accepted, the linked issue gets closed automatically. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@cclauss cclauss requested a review from rohan472000 September 23, 2023 12:13
Copy link
Contributor

@rohan472000 rohan472000 left a comment

Choose a reason for hiding this comment

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

LGTM

@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass labels Sep 23, 2023
@cclauss
Copy link
Member

cclauss commented Sep 23, 2023

I added doctests that fail...

=================================== FAILURES ===================================
___________ [doctest] strings.knuth_morris_pratt.knuth_morris_pratt ____________
009     1) Preprocess pattern to identify any suffixes that are identical to prefixes
010 
011         This tells us where to continue from if we get a mismatch between a character
012         in our pattern and the text.
013 
014     2) Step through the text one character at a time and compare it to a character in
015         the pattern updating our location within the pattern if necessary
016 
017     >>> kmp = "knuth_morris_pratt"
018     >>> knuth_morris_pratt(kmp, "kn") == kmp.find("kn")
Expected:
    True
Got:
    False

As discussed in CONTRIBUTING.md, you can run these on your own computer with:
python3 -m doctest -v strings/knuth_morris_pratt.py

@rohan472000
Copy link
Contributor

while testing the doctests added by @cclauss with existing code of knuth_morris_pratt.py in repo, it is also failing.

@Akashram28
Copy link
Author

Akashram28 commented Sep 23, 2023

image

knuth_morris_pratt The pattern is the first parameter, the text is the second. Or am I doing something wrong ? Sry this is my first PR

@Akashram28 Akashram28 closed this Sep 23, 2023
@Akashram28 Akashram28 reopened this Sep 23, 2023
@algorithms-keeper algorithms-keeper bot added the enhancement This PR modified some existing files label Sep 23, 2023
@cclauss
Copy link
Member

cclauss commented Sep 24, 2023

If we are going to be consistent with str.find() then we should change knuth_morris_pratt(sub, text) to knuth_morris_pratt(text, sub) to match the str.find() calling conventions and knuth_morris_pratt(text, sub) == text.find(sub) should always be True.

@Akashram28 Akashram28 deleted the kmp_return_index branch October 1, 2023 11:54
@Akashram28 Akashram28 restored the kmp_return_index branch October 1, 2023 11:54
@Akashram28 Akashram28 deleted the kmp_return_index branch October 1, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files tests are failing Do not merge until tests pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning index instead of boolean when knuth_morris_pratt matches.
4 participants