Mistake in maths/average_mode.py fixed.#4464
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
mrmaxguns
left a comment
There was a problem hiding this comment.
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. <49735721+mrmaxguns@users.noreply.github.com>
* 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
mrmaxguns
left a comment
There was a problem hiding this comment.
Thanks for the changes. Here is my review.
| 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.
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. <49735721+mrmaxguns@users.noreply.github.com>
|
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. <49735721+mrmaxguns@users.noreply.github.com> * 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. <49735721+mrmaxguns@users.noreply.github.com> * Suggestions added Co-authored-by: Maxim R. <49735721+mrmaxguns@users.noreply.github.com>
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}.