128. Longest Consecutive Sequence by t-ooka · Pull Request #7 · t-ooka/leetcode

t-ooka · 2024-07-02T05:39:31Z

問題: https://leetcode.com/problems/longest-consecutive-sequence

sakzk · 2024-07-03T07:00:59Z

Longest Consectuive Sequence/step1.py

+        num_to_index = {}
+
+        for i in range(0, len(nums)):
+            num_to_index[nums[i]] = i


num_to_index のバリューは最後まで使われていないですね。
set が思いつかなかったとしたら、num_to_index[nums[i]] = None などとすると、インデックスを使わないことのシグナルになると思います。

以下は set の内部実装に関して見つけたものです。
https://stackoverflow.com/questions/3949310/how-is-set-implemented

レビュー頂き誠にありがとうございます。
step1で解法思いつかず試行錯誤してたところで使われていない値があることに気づけていませんでした。こういった点意識して再度コーディングをしてみます！

sakzk · 2024-07-03T07:01:36Z

Longest Consectuive Sequence/step1.py

+
+        for key in num_to_index.keys():
+            count = 1
+            while key+1 in num_to_index:


算術演算子の周りにスペースを入れるかどうかは、PEP8のサンプルを見る限りどちらでもいいみたいです。ひとつのPRで混ざっていると、なにか使い分けがあるのかな？と少し注意が向くのでどちらかに統一してあるといいかもです。
https://peps.python.org/pep-0008/#other-recommendations
discord のログには次のような議論が見つかりました。他の方がどのような判断をしているかの参考までに。
見た目が窮屈になるときに開けるかどうか：https://discord.com/channels/1084280443945353267/1225849404037009609/1254109293091881052

算術演算子周りのスペースについて別のところでもご指摘を頂いておりました。ご指摘を頂く前にやったコーディングでごちゃごちゃになってしまってたと思います。今後はスペースを空ける方向で統一しようと思います。

sakzk · 2024-07-03T07:08:45Z

Longest Consectuive Sequence/step2-using-set.cpp

+class Solution {
+public:
+    int longestConsecutive(vector<int>& nums) {
+        unordered_set<int>num_set(nums.begin(), nums.end());


自分はC++で書いていないので詳しくは知らないのですが、set関連のデータ構造はいろいろあるようです。
Ryotaro25/leetcode_first60#14 (comment)

内部実装、各種操作の計算量がPythonと異なる点も意識が必要と思われます。
Ryotaro25/leetcode_first60#14 (comment)

上記読む限り、vector を unorderd_set に変換する操作は O(n) で実行できなさそうです。

ハッシュセット・ハッシュテーブルへの挿入は、O(1)と考えて問題ないかと思います。

sakzk · 2024-07-03T07:17:38Z

Longest Consectuive Sequence/step2-using-set.py

+                while num+length in num_set:
+                    length+=1
+
+                longest = max(length, longest)


これは好みの問題ですが、自分は上下のスペースは削ったほうが見やすく感じます。

好みのところでご意見いただけること、とても参考になります。ありがとうございます！

sakzk · 2024-07-03T07:33:05Z

Longest Consectuive Sequence/step3.cpp

+
+        for ( auto &n: nums) {
+            if (!num_set.contains(n-1)) {
+                int length = 1;


nums の要素の値の元の型は、関数の引数まで読み返さないとわからないので、&n の型を int に明示して、int length と揃えてもいいのかなと思いました。

一応、nums の要素数が大きい場合に、length がオーバーフローしないかは気になりました。だいたい 2* 10^9でオーバーフローしますが、扱うデータの大きさによってはあり得る状況かなと思います。(暗黙の型変換などされるのでしょうか？)

以下、int型の取りうる範囲についての参照リンクです。
https://learn.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-170
https://en.cppreference.com/w/cpp/types/climits

sakzk · 2024-07-03T07:35:23Z

Longest Consectuive Sequence/step3.py

@@ -0,0 +1,14 @@
+class Solution:
+    def longestConsecutive(self, nums: List[int]) -> int:
+        num_set = set(nums)


変数名 nums_set は、自分の場合は「集合だから重複は含まれていないんだな」と一段回思考を挟むことがあります。unique_nums とかもありだと思います。

sakzk · 2024-07-03T07:36:39Z

Longest Consectuive Sequence/step3.py

+    def longestConsecutive(self, nums: List[int]) -> int:
+        num_set = set(nums)
+
+        longest_consecutive_sequence = 0


変数名 longest_consecutive_sequence からは、何らかの連続そのものを想起します。
長さの最大値を保存する変数の名前としてはlongest_length, max_length あたりが良いのではないかと思います。
名前のスコープがもっと長いなら longest_consecutive_sequence_length も選択肢になるかもしれません。
(調べた限りでは、sequence に「長さ」の意味は見つけられませんでした。
https://dictionary.cambridge.org/dictionary/english/sequence
命名に使う英単語を辞書を引いてみることは、自分もよくやっているのですがおすすめです。変数名に使う英語はそれほどパターンが多くないのでだんだん網羅されている感じがあります。)

sakzk · 2024-07-03T07:42:05Z

Longest Consectuive Sequence/step3.py

+
+        longest_consecutive_sequence = 0
+
+        for num in nums:


すでに検討されていたなら申し訳ないのですが、step1 とstep4の時間計算量、空間計算量は見積りましたか？

sakzk · 2024-07-03T07:44:42Z

Longest Consectuive Sequence/step4-using-set.py

@@ -0,0 +1,13 @@
+class Solution:
+    def longestConsecutive(self, nums: List[int]) -> int:
+        nums_set = set(nums)


おまけ問題として、重複も連続として数えるとした場合にどのように変更するか考えてみるのも面白いかもです。

sakzk · 2024-07-03T07:57:16Z

Longest Consectuive Sequence/step4-using-set.cpp

+        unordered_set nums_set(nums.begin(), nums.end());
+        int longest_consecutive_sequence = 0;
+
+        for ( auto num: nums_set) {


すでにご存知でしたら申し訳ないのですが、参照渡しをするか、コピーをするか、コピーする場合はコストがかかることを意識しているかに関して discord 上で以下のような議論がありました。ご参考までに。
C++の例：https://discord.com/channels/1084280443945353267/1237649827240742942/1251719996242002043
pythonの例：
hrkh/leetcode#2 (comment)

これ、int の場合は大きさが32ビットで、あんまりリファレンスにする必要は感じません。コピーコストが安いからですね。

sakzk · 2024-07-03T14:13:33Z

Longest Consectuive Sequence/step4-using-set.cpp

+        unordered_set nums_set(nums.begin(), nums.end());
+        int longest_consecutive_sequence = 0;
+
+        for ( auto num: nums_set) {


フォーマットに関して discord 内でよく引用されている印象がある google C++ style guide では、for () の () 前後にスペースを入れない書き方が推奨されていました。
https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching

色々なところでスペースがあったりなかったり一貫性がないのが気になりますね。普通は開きカッコの後ろにスペースを入れるなら、閉じカッコの前にも入れるかと思います。sakzkさんのコメントのようにない方が一般的かと思います。

liquo-rice · 2024-07-03T15:25:43Z

Longest Consectuive Sequence/step4-using-set.cpp

+        unordered_set nums_set(nums.begin(), nums.end());
+        int longest_consecutive_sequence = 0;
+
+        for ( auto num: nums_set) {


色々なところでスペースがあったりなかったり一貫性がないのが気になりますね。普通は開きカッコの後ろにスペースを入れるなら、閉じカッコの前にも入れるかと思います。sakzkさんのコメントのようにない方が一般的かと思います。

liquo-rice · 2024-07-03T15:26:27Z

Longest Consectuive Sequence/step3.cpp

+class Solution {
+public:
+    int longestConsecutive(vector<int>& nums) {
+        unordered_set<int> num_set(nums.rbegin(), nums.rend());


どうしてreverse_iteratorですか？

liquo-rice · 2024-07-03T15:30:10Z

Longest Consectuive Sequence/step3.cpp

+            if (!num_set.contains(n-1)) {
+                int length = 1;
+                while (num_set.contains(n + length)) {
+                    length += 1;


liquo-rice · 2024-07-03T15:34:44Z

Longest Consectuive Sequence/step2-using-set.cpp

+class Solution {
+public:
+    int longestConsecutive(vector<int>& nums) {
+        unordered_set<int>num_set(nums.begin(), nums.end());


ハッシュセット・ハッシュテーブルへの挿入は、O(1)と考えて問題ないかと思います。

liquo-rice · 2024-07-03T15:36:55Z

Longest Consectuive Sequence/step4-using-set.cpp

+class Solution {
+public:
+    int longestConsecutive(vector<int>& nums) {
+        unordered_set nums_set(nums.begin(), nums.end());


ここだけtemplate argument deductionを使っていますが、意識してやっていますか？

oda · 2024-07-04T16:29:14Z

Longest Consectuive Sequence/step4-using-set.py

+        nums_set = set(nums)
+        longest_consecutive_sequence = 0
+
+        for num in nums_set:


なんとなく不自然さを感じます。つまり、手でやるとして、{0, 1, 2, 3, 4, 5} だったとして、0 から確認が終わった後に 1 を確認しないじゃないですか。

t-ooka · 2024-07-05T07:21:11Z

@sakzk @liquo-rice @oda
レビューいただき本当にありがとうございます。一個一個確認していってますのでまばらな返信についてはお許しください 🙇

t-ooka added 8 commits June 5, 2024 09:26

Create step1.py

3fc6b5f

Create step2-using-set.py

59ed418

Create step2-using-set.cpp

e5160fb

Create step3.py

f132f72

Create step3.cpp

375b69b

Update step3.py

597ec20

Create step4-using-set.py

8631568

Create step4-using-set.cpp

cf95edb

t-ooka changed the title ~~Question/longest consecutive sequence~~ 128. Longest Consecutive Sequence Jul 2, 2024

sakzk reviewed Jul 3, 2024

View reviewed changes

liquo-rice reviewed Jul 3, 2024

View reviewed changes

oda reviewed Jul 4, 2024

View reviewed changes

rihib mentioned this pull request Aug 30, 2024

Longest Consecutive Sequence rihib/leetcode#22

Closed

Conversation

t-ooka commented Jul 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakzk Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakzk Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-ooka commented Jul 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sakzk Jul 3, 2024 •

edited

Loading

sakzk Jul 3, 2024 •

edited

Loading