Skip to content

Mistake in maths/average_mode.py fixed. #4464

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

Merged
merged 10 commits into from
Jun 4, 2021

Conversation

haningrisha
Copy link
Contributor

Describe your change:

  • Mistake in maths/average_mode.py fixed. The previous solution was to return the
    value on the first loop iteration, which is not correct, more than that it
    used to delete repeating values, so result's array and check array lost
    relation between each other

  • Type hint added

  • 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 have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

the
value on the first loop iteration, which is not correct, more than that it
used to delete repeating values, so result's array and check array lost
relation between each other

* Type hint added
@haningrisha haningrisha requested a review from Kush1101 as a code owner May 29, 2021 22:25
@ghost ghost added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels May 29, 2021
@haningrisha haningrisha changed the title * Mistake in average_mode.py fixed. The previous solution was to return Mistake in maths/average_mode.py fixed. May 30, 2021
Copy link
Member

@mrmaxguns mrmaxguns left a comment

Choose a reason for hiding this comment

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

Thank you so much for this critical bug fix. The original mode function didn't actually return the mode of the input list, and instead always returned the first value of the list (since the return was inside the loop, thus happening at the first iteration).

The reason why the bug wasn't caught was because the value that was the mode always happened to be at the front of the list. More varying tests should prevent future mistakes like this.

Here is my review. If you disagree with any of the points, feel free to comment on them.

@ghost ghost added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels May 30, 2021
@ghost ghost added awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass and removed awaiting changes A maintainer has requested changes to this PR labels May 31, 2021
* output typing changed to Any
* test cases added
@mrmaxguns
Copy link
Member

Pre-commit failed because Black would reformat the file. Make sure to run black with black your_file.py. Black can be installed with pip install black.

@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label May 31, 2021
@haningrisha
Copy link
Contributor Author

haningrisha commented May 31, 2021

I was thinking about raising an exception in case the list has several modes(for instance [2,2,1,1]) several modes existence fact may be crucial in some algorithms. Also, I can add another function for working with multiple mode cases.

Example of usage:

try:
    m = mode(some list)
    # logic
except SeveralModeException:
    m = multiple_modes(some list)
    # logic with list

Or I can return a list even if there's only one mode, but this seems odd.

What do you think?

@mrmaxguns
Copy link
Member

I was thinking about raising an exception in case the list has several modes(for instance [2,2,1,1]) several modes existence fact may be crucial in some algorithms. Also, I can add another function for working with multiple mode cases.

Example of usage:

try:
    m = mode(some list)
    # logic
except SeveralModeException:
    m = multiple_modes(some list)
    # logic with list

Or I can return a list even if there's only one mode, but this seems odd.

What do you think?

Personally, I like the one-function approach. A second function that performs almost the same task seems redundant. There could just be one function which always returns a list. That way, the user can check the length by themselves. This also means that the function could return an empty list if the input list is empty.

File formatted
@ghost ghost added the awaiting reviews This PR is ready to be reviewed label Jun 1, 2021
statistics only used in doctest, now they are imported in doctest
@ghost ghost removed the tests are failing Do not merge until tests pass label Jun 1, 2021
Copy link
Member

@mrmaxguns mrmaxguns left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Here is my review.

Comment on lines 28 to 29
result = [input_list[i] for i, value in enumerate(result) if value == y]
result = list(set(result)) # Deletes duplicates
Copy link
Member

@mrmaxguns mrmaxguns Jun 4, 2021

Choose a reason for hiding this comment

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

Instead of creating a list, then converting it to a set, and then back to a list, you could do a set comprehension. Also, sorted takes any iterable, so passing a set to it (instead of converting it to a list first) will work.

Suggested change
result = [input_list[i] for i, value in enumerate(result) if value == y]
result = list(set(result)) # Deletes duplicates
result = {input_list[i] for i, value in enumerate(result) if value == y}

@ghost ghost added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Jun 4, 2021
@ghost ghost added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Jun 4, 2021
@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label Jun 4, 2021
@mrmaxguns
Copy link
Member

Thank you for contributing 🎉

@mrmaxguns mrmaxguns merged commit 40e357f into TheAlgorithms:master Jun 4, 2021
shermanhui pushed a commit to shermanhui/Python that referenced this pull request Oct 22, 2021
A serious bug was addressed with this pull request. The mode function previously
didn't return the mode of the input list. Instead, it always returned the first value.
Due to lacking tests, the bug was never caught. This pull request also adds new
functionality to the function, allowing support for more than one mode.

See TheAlgorithms#4464 for details.

* * Mistake in average_mode.py fixed. The previous solution was to returnthe
value on the first loop iteration, which is not correct, more than that it
used to delete repeating values, so result's array and check array lost
relation between each other

* Type hint added

* redundant check_list deleted

Co-authored-by: Maxim R. <[email protected]>

* Suggestions resolved

* output typing changed to Any
* test cases added

* Black done

File formatted

* Unused statistics import

statistics only used in doctest, now they are imported in doctest

* Several modes support added

Several modes support added

* Comment fix

* Update maths/average_mode.py

Co-authored-by: Maxim R. <[email protected]>

* Suggestions added

Co-authored-by: Maxim R. <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants