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

Most common element #29

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Most common element #29

merged 3 commits into from
Aug 29, 2024

Conversation

gudlot
Copy link
Contributor

@gudlot gudlot commented Aug 27, 2024

This is my suggestion for an implementation of the function most_common_element with pure Python commands, not using numpy. :-)
A test was provided.

Copy link
Collaborator

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, if we were more series, I would expect improving with Counter, and likely have another iteration (for some other nitpicky/small thing).

As is, I think this is a great PR start. (A PR without any iteration is very rare even for year-long maintainers. Up to: "I am worried if something goes in without a comment").

So LGTM thanks you. I'll just apply the comment removal first.

counting[i] += 1
else:
counting[i] = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also use from collections import Counter to avoid the if clause even. But considering how basic the project, is I think this is a perfect solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the update. I was not aware of Counter.

def most_common_element(ll):
   
    if not isinstance(ll, list):
        raise ValueError("Input must be a list")
    
    c = Counter(ll)
    return c.most_common(1)[0][0]

What do you mean by "likely have another iteration (for some other nitpicky/small thing)", please? Happy to learn.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant that it is not uncommon for someone to have small (often tiny/silly) asks to improve things a bit more.

Copy link
Contributor Author

@gudlot gudlot Sep 1, 2024

Choose a reason for hiding this comment

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

Happy to improve, thank you for the input :-)

tests/test_inspect.py Outdated Show resolved Hide resolved
@seberg seberg merged commit 3f210e5 into Nikoleta-v3:main Aug 29, 2024
4 checks passed
@seberg
Copy link
Collaborator

seberg commented Aug 29, 2024

Ah, we could have added a Closes #XX to automatically fix the bug (and also link from it here to make it easier to find).

@Nikoleta-v3
Copy link
Owner

We could have added them but do we want to close the issues? Just in case we use the material again in the future.

@seberg
Copy link
Collaborator

seberg commented Aug 29, 2024

Ah, good point. I closed the issues now... Maybe it makes sense to just reopen them anyway also (since they are linked, etc.)?

@Nikoleta-v3
Copy link
Owner

Sounds good to me!

Nikoleta-v3 added a commit that referenced this pull request Sep 4, 2024
Most common element - update for #29
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.

3 participants