-
-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Conversation
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
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.
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.
Co-authored-by: Maxim R. <[email protected]>
* output typing changed to Any * test cases added
Pre-commit failed because Black would reformat the file. Make sure to run black with |
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:
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
statistics only used in doctest, now they are imported in doctest
Several modes support added
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.
Thanks for the changes. Here is my review.
maths/average_mode.py
Outdated
result = [input_list[i] for i, value in enumerate(result) if value == y] | ||
result = list(set(result)) # Deletes duplicates |
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.
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.
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} |
Co-authored-by: Maxim R. <[email protected]>
Thank you for contributing 🎉 |
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]>
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:
Fixes: #{$ISSUE_NO}
.