-
Notifications
You must be signed in to change notification settings - Fork 0
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
Longest Consecutive Sequence #22
base: main
Are you sure you want to change the base?
Conversation
numsMap[n] = struct{}{} | ||
} | ||
maxLength := 0 | ||
for _, n := range nums { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ1800msと90msがめちゃくちゃ面白くて、実はdeleteしなくてもrangeにnumsを使うかnumsMapを使うかでもこの差が出ます。
pythonでもほぼ同じ現象が再現して、pythonの場合は4200msぐらいと350msぐらいに計算時間分布の山がありますが、delete(remove)しなくても、引数のnumsを直接使ってイテレーションするか構成したmapなりsetなりを使うかだけでこの差が出ます。
この差のメカニズムがわかっていないのですが、numsMapのイテレーションだとnを取り出した直後にn-1が含まれるか否かの判定をするので判定しやすい構造になっていて、numsのイテレーションだとn-1の判定のしやすさがランダムになっているので判定しにくい、みたいな事なのかな?と思いました。
deleteしなくてnumsMapでのイテレーションと、deleteしてnumsでのイテレーションがほぼ同じになるのは、deleteでnumsMapの構造がなんらか最適化されて、次のn-1の判定がしやすくなっているのかな...どういうメカニズムなんだろう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。確かにdeleteしなくてもmapに変えるだけで90msぐらいになりますね。少し考えてみたのですが、問題には重複する要素が含まれないとは書かれていないので、案外重複する要素(特に連続する値の先頭の要素)の多い配列の場合、毎回同じ処理をすることになるので遅くなるみたいな単純な理由もありそうだなと思いました
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あーそうか、n-1が入ってなかったら普通に処理しちゃうのか。
当たり前でしたね。すみません。。。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
僕も完全にdeleteのおかげで速くなったと思ってしまっていて、そういえばmapによって重複がなくなるなということに気づけてなかったので、ご指摘いただけてありがたいです🙏
length := 1 | ||
for { | ||
n++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このループの外の n を直接変化させているので、ループを抜けた後に n を扱う処理がある場合にバグが発生しやすいのでは、と思いました。(今回のケースではループを抜けた後には maxLength の更新だけなので問題なさそうです)
このループの前あたりに length と合わせてインクリメントする変数を宣言して n 自体とは区別するといいかもです。(適当に cur としましたが、これはいい変数名ではなさそうです)
length := 1 | |
for { | |
n++ | |
cur := n | |
length := 1 | |
for { | |
cur++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n+length
とするとn自体をインクリメントせずに表現できそうですね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
別の言い方をすると、一文字変数は説明書きが長くなるような使い方をするべきではない、ということかなと思います。
for n := range numsMap
とあったら、普通は「numsMap のキー」と理解するし、それ以上の理解はしないので、間で別のものにすり替えられていると見落とす、ということです。
他の人のPRを見て、「手でやるとして、{0, 1, 2, 3, 4, 5} だったとして、0 から確認が終わった後に 1 を確認しないじゃないですか」(https://github.com/t-ooka/leetcode/pull/7/files#r1665902244 )というodaさんのコメントを発見し、確かにその通りだと思ったので下記のように一度連続する数に含めたものは削除するように変更した。 | ||
実際にLeetCode上でもStep1の場合、5回ほど提出しても実行時間がどれも1800msほどだったのに対し、Step2の場合は90msぐらいだったので、実際にとても早くなることが実感できた。 | ||
|
||
質問:どうやらこの問題はUnion FIndを使っても解くことができるようですが、Union FIndを使って解く練習もするべきでしょうか? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
どちらにしてもどこかでは Union Find をやることになると思いますが、ここでなくてもいいかもしれませんね。
(目的は、できるようになることで、こなすことではないのです。)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。Union Findが常識に含まれるのかがわからなかったのですが、いずれやることになるのであれば含まれていそうですね
Longest Consecutive Sequenceを解きました。レビューをお願いいたします。
問題:https://leetcode.com/problems/longest-consecutive-sequence/
言語:Go
すでに解いている方々:
sakzk/leetcode#2
t-ooka/leetcode#7