Skip to content

Top K Frequent Elements #10

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Top K Frequent Elements #10

wants to merge 1 commit into from

Conversation

bumbuboon
Copy link
Owner

Copy link

@t0hsumi t0hsumi left a comment

Choose a reason for hiding this comment

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

全体を通して、簡潔にさまざまな解法が試されていて読みやすかったです。

```python
class Solution:
def topKFrequent(self, nums: List[int], k: int) -> List[int]:
def quick_select(left, right):
Copy link

Choose a reason for hiding this comment

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

好みの問題だと思いますが、僕ならquick_select()の引数をlist, left, right, pivot_indexの4つになる様に書くかなと思いました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

今回は以前に書いていた方のを写しただけでして、そこまでは思い至りませんでした。
自分で0から書くときはその方法も検討させていただきます。

count[num] = 1

count_sorted = sorted(count.items(), reverse=True, key=lambda x:x[1])
k_elements = [count_sorted[i][0] for i in range(k)]
Copy link

Choose a reason for hiding this comment

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

これは単にスライス[:k]でよいでしょう。

Copy link
Owner Author

Choose a reason for hiding this comment

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

確かにその通りです。
書いた当時は少し複雑に考えすぎていました。

- 負のカウントも取れる
- 独自メソッド
- elements
- most_common
Copy link

Choose a reason for hiding this comment

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

GitHub に CPython のソースがあるので、実装を読んでみてもいいでしょう。読み始めるのはもう少し先でもいいです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。
自分にはまだ難易度が高いですがチャレンジすることも意識してみようと思います。

Comment on lines +200 to +204
num_to_count = {}
for num in nums:
if num not in num_to_count:
num_to_count[num] = 0
num_to_count[num] += 1
Copy link

@Hurukawa2121 Hurukawa2121 Dec 18, 2024

Choose a reason for hiding this comment

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

未処理のキーに対応するデフォルト値は defaultdict を使うと指定できます。

num_to_count = defaultdict(int)
for num in nums:
    num_to_count[num] += 1

Copy link
Owner Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。defaultdictは検討しましたが、中身をきちんと理解していないので今回は使わないこととしました。

partition_index += 1
values[right], values[partition_index] = values[partition_index], values[right]
if partition_index == k - 1:
return
Copy link

Choose a reason for hiding this comment

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

同じ関数の中で値を返さない return と返す return が混在しているのが気になりました。この関数は出力として値を返さないため、値を返さないほうに統一するとよいと思います。

if partition_index == k - 1:
    return
elif partition_index < k - 1:
    quick_select(partition_index + 1, right)
elif partition_index > k - 1:
    quick_select(left, partition_index)

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。
出力を返すかどうかはあまり考えていなかったので参考になります。

Comment on lines +89 to +90
- (key:value)をどうやってheap queにすればいいのかわからない。
- (value, key)のtupleにしてheap queに追加する

Choose a reason for hiding this comment

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

細かいですが、キューのスペルは queue です

Comment on lines +181 to +182
if len(result) >= k:
return result[:k]

Choose a reason for hiding this comment

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

num の種類が k 未満の場合には、return に到達せずに for ループを終えるので None を返してしまいそうです。
k 未満の場合の対応をどうするかにもよりますが、他の実装(L104 return [value for _, value in frequent_values] や L212 return [num for _, num in top_k_heap])ではある分だけ返すようになっていたので、それに合わせるとこんな感じがよいかもです。またその場合にはスライスせず result で十分そうです。

Suggested change
if len(result) >= k:
return result[:k]
if len(result) >= k:
break
return result

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.

6 participants