Skip to content

347. Top K Frequent Elements#10

Merged
Ryotaro25 merged 3 commits intomainfrom
problem9
Nov 9, 2024
Merged

347. Top K Frequent Elements#10
Ryotaro25 merged 3 commits intomainfrom
problem9

Conversation

@Ryotaro25
Copy link
Copy Markdown
Owner

問題へのリンク
https://leetcode.com/problems/top-k-frequent-elements/description/
問題文(プレミアムの場合)

備考

次に解く問題の予告
373. Find K Pairs with Smallest Sums

フォルダ構成
LeetCodeの問題ごとにフォルダを作成します。
フォルダ内は、step1.cpp、step2.cpp、step3.cpp、priority_queue.cpp、quick_select.cppとmemo.mdとなります。

memo.md内に各ステップで感じたことを追記します。

各要素のそれぞれの数を計算する段階では順番は関係ないのでunordered_mapを使う
コード量は増えるがfirst、secondが何を表すのか明示した方が処理を理解しやすいと思った

## 他の解法
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

他の解法として、Bucket sort で実装してみるのもありかと思いました。
(C++ の経験がほぼないので、コードへのコメントは他の方にお任せしようと思います)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@thonda28 step7.cppに追加しました🙇

要素が重複しない場合は入力データの大きさに影響を受ける

## ステップ2
自分の変数はcountを使っていたがfrequencyを使う方が伝わりやすそう
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

僕も最初はcountつかってましたが、同じくfrequencyのほうがいいと思いました。

わかりにくい。kの数までの方が素直と感じた

## Discordや他の人のコードなど
クイックセレクトも常識として知っておく
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

~kthな要素を取り出すときはquickselectがあるというのが常識そうです。

https://en.wikipedia.org/wiki/Quickselect

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@t-ooka
コメントありがとうございます。
quickselectが使われるときはまさにこういったときなのですね。知りませんでした。ありがとうございます!

class Solution {
public:
vector<int> topKFrequent(vector<int>& nums, int k) {
unordered_map<int,int>nums_frequency;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

変数名のnums_frequencyですが、整数に対して頻度なので、num_to_freqencyとかのほうが分かりやすいかと思いました。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@t-ooka step5.cppに修正版を追加しました。

nums_frequency[num]++;
}

priority_queue<pair<int,int>>descending_numbers;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

max heapを初期化しているとおもうのですが、降順という意味合いよりもそのままmax_heap_numbersとかでもいいのかなと思いました。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@t-ooka
ありがとうございます。
他の指摘事項を踏まえてstep5.cppに修正版を追加しました。

}

vector<pair<int, int>> descending_numbers;
for (auto num : nums_counts) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for (const auto [num, count] : nums_counts) {

こういう風にも書けますね
https://en.cppreference.com/w/cpp/language/range-for

range-declaration may be a structured binding declaration:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo こちらの書き方失念しておりました。きちんと覚えておきます。

for (auto num : nums_counts) {
descending_numbers.push_back({num.second, num.first});
}
sort(descending_numbers.rbegin(), descending_numbers.rend());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sort(descending_numbers.begin(), descending_numbers.end(), greater<pair<int, int>>());

これの方が素直なのかな、という気がします

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo レビューありがとうございます。rbegin()とrend()はあまり使わないのでしょうか?🙇

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

そんなことはないと思うのですが、sortで降順に並べたかったらまず第3引数を指定するかな、という感覚が自分にはあったのでコメントしました。
たとえば逆順にiterateしたいときに for (auto it = foo.rbegin(); it != foo.rend(); it++) と書くのは自然な気がします。
ただ、そんなにC++経験ないので自信はないです 🙇

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo
今回のように降順と明示している場合には第三引数で記述するように致します。
自分はLeetCode以外で触ったことないので助かります🙇コメントありがとうございました。

c++には相当するものがなさそうだけどmapにを使えば実装はできる(実際にstep1で使った方法)
https://github.com/t-ooka/leetcode/pull/3/files#diff-e14e39d01f77a7c369b1f688b2e0c521b4640ce25ca0db0a4a17e789cac614f3

各要素のそれぞれの数を計算する段階では順番は関係ないのでunordered_mapを使う
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

https://discord.com/channels/1084280443945353267/1206101582861697046/1240515165582135336

unordered_map は実は遅いという話があります。

https://chromium.googlesource.com/chromium/src/+/master/base/containers/README.md#Map-and-set-selection

Do not default to using std::unordered_set and std::unordered_map. In the common case, query performance is unlikely to be sufficiently higher than std::map to make a difference, insert performance is slightly worse, and the memory overhead is high. This makes sense mostly for large tables where you expect a lot of lookups.

この辺の話があるので、単純に順番が関係ないからunordered_map, という考えだとむしろ遅くなるケースもありそうです。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo コメントありがとうございます。unordered_mapは必ずしもmapに比べて早いというわけでは無いのですね。知りませんでした🙇ありがとうございます。

nums_frequency[num]++;
}

priority_queue<pair<int,int>>descending_numbers;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Min Heapを使ってサイズがkより大きくなったら最小をpopするを繰り返す、という方法もありそうです。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo
step8.cppに追加いたしました。

return sorted_i;
}

void quickselect(vector<pair<int, int>>& counts, int left, int right, int kth_smallest) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

クラス内部でしか使わなさそうに見えるので、privateにして露出を防いでも良さそうです
(partition()も同様)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo この辺りこれまで意識していなかったので以降気をつけます。

Comment on lines +38 to +43
unordered_map<int, int> frequency_numbers;
for (int num : nums) {
frequency_numbers[num]++;
}

vector<pair<int, int>> counts_numbers(frequency_numbers.begin(), frequency_numbers.end());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

frequency_numbersとcounts_numbersという名前は微妙な気がします。
この部分を関数化して、中でmapを作ってからvectorに変換して返すようにするのはどうでしょうか。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo 関数化してみました。step9.cppに修正版をアップしました。

if (kth_smallest == pivot_index) {
return;
} else if (kth_smallest < pivot_index) {
quickselect(counts, left, pivot_index - 1, kth_smallest);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

あえて再帰しなくても、leftとrightを更新するだけなのでループで十分な気がしました。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo step9.cppに修正版をアップしました。


vector<int> top_k_numbers;
for (int i = number_count - k; i < number_count; ++i) {
int element = counts[i].first;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

countsという変数はこのスコープにないと思います

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@fhiyo 誤ったものあげておりました。失礼しました。step9.cppに修正版をアップしました。

@TORUS0818
Copy link
Copy Markdown

みなさんがしっかりコメント付けてくださったので、追加コメントは特にないのですが、強いて言うなら、それぞれの解法のプロコン比較のようなものもあると良いのかなと思いました(私もあんまりできてないのですけど、、)

今回の例で言えば、深さkで止めたMinHeapやquickselect使えば時間空間計算量はどうなるか(実際の速度はどうか)、アルゴリズムとしてわかりやすいものになるのか、それらを踏まえて今回のケースではどれが適切なのかの言及まであると、色々議論が盛り上がると思いました。

@Ryotaro25
Copy link
Copy Markdown
Owner Author

@thonda28 @t-ooka @fhiyo @TORUS0818
皆様レビューありがとうございました。理解と修正に時間がかかり申し訳ございません。
今ほどPushしました。

@Ryotaro25
Copy link
Copy Markdown
Owner Author

@TORUS0818 いったん計測のところまでまとめました。どれが適しているのか調べきれておりませんので後ほど追加します。

@Ryotaro25 Ryotaro25 merged commit 9f46b58 into main Nov 9, 2024
## 各実装の比較
計測値はleetcodeを使用
**priority_queue(step5.cpp)**
時間計算量:O(n)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Priority QueueのPush, Pop時には、logNの時間計算量が必要なので、
Push時は、N回 logNの操作を行うので、O(n log n)だと思います。

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