-
Notifications
You must be signed in to change notification settings - Fork 0
695 max area of island medium #14
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
base: main
Are you sure you want to change the base?
Conversation
| num_columns = len(grid[0]) | ||
| ISLAND = 1 | ||
|
|
||
| def is_island(cell: tuple[int, int]) -> bool: |
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.
step2 のように row と column を渡したほうが個人的には 分かりやすいと感じました。
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.
レビューありがとうございます。
そうですね、タプルにしてcellをオブジェクトらしくするか、int2つで管理するかは非常に迷いました。
結局、(プロダクトでは適当なクラスを定義するでしょうし、)コードが完結になり、オブジェクト指向らしくてわかりやすいタプルを選びましたが、�LeetCodeの解答としてはintが適切かもしれないですね。感想ありがとうございます。
| def is_island(row: int, column: int) -> bool: | ||
| return is_valid(row, column) and grid[row][column] == ISLAND | ||
|
|
||
| def _calculate_area_bfs( |
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.
関数名には実装の詳細や使用されているアルゴリズムより、その関数からどのような値が返ってくるかが書かれていて欲しいです。 _bfs は取り除いてよいと思います。
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.
ありがとうございます。以後気をつけます
| row, column = cells_to_visit.popleft() | ||
| for dr, dc in DIRECTIONS: | ||
| neighbor = (row + dr, column + dc) | ||
| if is_island(*neighbor) and neighbor not in visited_cells: |
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.
真偽を反転し、 continue し、ネストを下げたほうが読みやすくなると思います。
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.
以下のようにするということですよね。ありがとうございます。
if not is_island(*neighbor): continue
if neighbor in visited_cells: continue
...
このあたりは、いつも迷うところなのですが、何か具体的な方針などあるのでしょうか。今回のコードは↓のトレードオフがあるのかなと感じて、私は1を選択しました。
- 直接、条件を指定する方が、否定や余事象で指定するよりわかりやすい
- ネストは浅い方がわかりやすい
| row, column = cells_to_visit.popleft() | ||
| for dr, dc in DIRECTIONS: | ||
| neighbor = (row + dr, column + dc) | ||
| if is_island(*neighbor) and neighbor not in visited_cells: |
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.
タプルを関数の引数にアンパックして渡すのは、やろうとしていることに対して無用な複雑性を加えているように感じました。 step2_inplace などのように、 row_neighbor と column_neighbor に分解した状態で保持し、 visited_cells や cells_to_visit に入れる段階でタプルにしたほうが読みやすいと思います。
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.
わかりました。固定長で二つくらいならアンパックは確かにやり過ぎ感があるのですね。指摘くださりありがとうございます。
| def is_island(row: int, column: int) -> bool: | ||
| return is_valid(row, column) and grid[row][column] == ISLAND | ||
|
|
||
| def _calculate_area_bfs( |
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.
inner function の関数名の先頭には _ は付けないことが多いと思います。他のモジュールやクラスから隠したい関数などは、先頭に _ を付けることが多いと思います。
https://google.github.io/styleguide/pyguide.html#3162-naming-conventions
Prepending a single underscore (_) has some support for protecting module variables and functions (linters will flag protected member access). Note that it is okay for unit tests to access protected constants from the modules under test.
|
|
||
| 2. 探索アルゴリズムの変更: | ||
| * `step1`: `list`をスタックとして利用し、`pop()`で要素を取り出す深さ優先探索(DFS)もどきを反復処理で実装していました。 | ||
| * `step2`: `collections.deque`をキューとして利用し、`popleft()`で要素を取り出す幅優先探索(BFS)を実装しています。アルゴリズムとしては何でもいい(連結成分さえ取り出せればOK)ですが、BFSの方が有名で、読み手が理解しやすいと考えました。 |
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.
BFS と DFS は同じくらいの知名度かなと思いました。
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.
すみません、書き方が良くなかったです。ここでstackを用いるだけでは厳密にはDFSにならず、DFSもどきになるので、そうなるくらいなら典型的なBFSにした方が読み手がわかりやすいかなと考えた、の意です。
| if not is_island(initial_island): | ||
| return 0 | ||
|
|
||
| cells_to_visit = deque([initial_island]) |
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.
自分なら frontier と explored と名付けるのですが、趣味の範囲だと思います。
| cell = (row, column) | ||
| if is_island(cell) and cell not in visited_cells: | ||
| area = calculate_area_bfs(cell, visited_cells) | ||
| max_area = max(max_area, area) |
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.
個人的には199行目あたりは以下のように書くのもいいと思っています。
if文として覚えておくべき条件を減らしたいからです。
また、ループでやりたいことはareaの最大サイズの更新なので、「ある条件の時だけ更新する」というより「ある条件の時は更新を行わない」の方が個人的には自然な気がしています。
cell = (row, column)
if not is_island(cell):
continue
if cell in visited_cells:
continue
area = calculate_area_bfs(cell, visited_cells)
max_area = max(max_area, area)ここらへんの話が近い話だと思っています。
https://discord.com/channels/1084280443945353267/1200089668901937312/1207200647594639391
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.
レビューありがとうございます。
皆さんのコメントも見ながら、こちらの方がコードを読んでいるときのワーキングメモリーの利用が確かに節約できるなという感覚がわかってきました。early returnなども含めて意識していきます。
| num_columns = len(grid[0]) | ||
| ISLAND = 1 | ||
|
|
||
| def is_island(cell: tuple[int, int]) -> bool: |
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.
is_unvisited_land()として、visitedかどうかもこの中で確認してしまう手もあるかなと思いました。
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.
レビューありがとうございます。
それも確かにそうですね。ただ、中身を覚えておく関数が多いと、ワーキングメモリーの利用量が上がるので、単純な処理なら前に出してしまいたい、というのもありますよね。そのあたりはトレードオフなんでしょうか。
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.
私は今回の状況はトレードオフにはなっていないかな、と感じでいるので、私の理解を記載してみます。
- 課題感:
- 該当箇所:
if is_island(*neighbor) and neighbor not in visited_cells- セットで2回繰り返し使われているのでまとめても良さそう
- 2つの条件を同時に考えているので、前の条件の結果を覚えたまま次のことを考える必要があり、脳のワーキングメモリを少し多めに使用する
- 該当箇所:
- 提案した対応案
- is_island と not in visited_cells をまとめて条件判定できる、
is_unvisited_land()を用意することで、条件判定を一つにしてしまう
- is_island と not in visited_cells をまとめて条件判定できる、
- 感じ方の齟齬がありそうに思っている部分
- is_unvisited_land()という明確な名前のついた関数ならば、中身の詳細を覚えておく必要はないのでワーキングメモリは使わない VS 複雑な関数を作ると中身を覚えておく必要がありワーキングメモリを使う
どういうものを読むと脳に負担を感じるかなど、個人差があるかもしれないので、あくまでそういう人もいるんだなと受け取っていただければと思います。
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.
丁寧にありがとうございます。(返信遅れてしまってごめんなさい)
そうですね。たしかにそこまできちんと考えてみると、is_unvisited_island 関数を入れてしまった方がわかりやすいと感じるようになりました。ありがとうございます。
| ) | ||
|
|
||
| def calculate_area_bfs( | ||
| initial_island: tuple[int, int], visited_cells: set |
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.
nits: islandという単語は島全体を表すので、initial_landやinitial_cellの方が近いかなと思いました。
An island or isle is a piece of land, distinct from a continent, completely surrounded by water.
https://en.wikipedia.org/wiki/Island
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.
それは知りませんでした。ご指摘ありがとうございます。
| 与えられた2次元のグリッドにおいて、1は陸地、0は水を表す。最大の島の面積を求める問題です。島は上下左右に隣接する1の集合として定義される。 | ||
|
|
||
| # 自分の解法 | ||
| 各マスを走査し、1が見つかったらそのマスを起点にグラフ探索を行い、島の面積を計算します。グラフ探索では連結成分を取り出せれば十分なので、走査の順番は特に気にしません。DFSでもBFSでも何でも解けますが、いずれにせよパフォーマンスを考えて、反復法で実装します。 |
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.
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.
確かにその通りですね。そちらでもやってみます。
| area = 1 | ||
|
|
||
| connected_islands = [(i,j)] | ||
| while connected_islands: |
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.
好みの範疇かもですが、ここはconnected_islandsの大きさが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.
レビューありがとうございます。
そうですね、グラフアルゴリズムではよく、(リストなりキューなりスタックなりが)空で無い限りループを回し、空になったらループを抜ける、と書く気がするので、そちらに合わせましたが、大きさ1以上と書いても良いのかもしれないですね。
|
|
||
| num_rows = len(grid) | ||
| num_columns = len(grid[0]) | ||
| ISLAND = 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.
定数なのでグローバルなど、関数ではない場所でで定義したいです。
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.
レビューありがとうございます。そうします。
| num_columns = len(grid[0]) | ||
| ISLAND = 1 | ||
|
|
||
| def is_island(cell: tuple[int, int]) -> bool: |
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.
cell よりは position の方が読みやすく感じるのですが、好みだと思います。そもそもでいうと、私なら row, col を引数にするかなと思います。
| ) | ||
|
|
||
| def calculate_area_bfs( | ||
| initial_island: tuple[int, int], visited_cells: set |
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.
initial_"island" が座標 (cell) を表すというのが不一致であるように感じます。下記コメントのように frontier / explored にするか、initial_row, initial_col、もしくは start_position のような命名にすると思います。
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.
確かにcell, row/columnに加えてinitial_islandも使うと混乱を生みかねないですね。修正した方がわかりやすそうだなと感じました。
| if is_island(neighbor) and neighbor not in visited_cells: | ||
| visited_cells.add(neighbor) | ||
| cells_to_visit.append(neighbor) |
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.
一旦全部 queue に入れて、pop したときに判定するという手もありますね。個人的にはその方がきれいに書けるような気がします。
…or max area of island solution
問題へのリンク
Max Area of Island - LeetCode
言語
Python
問題の概要
与えられた2次元のグリッドにおいて、1は陸地、0は水を表す。最大の島の面積を求める問題です。島は上下左右に隣接する1の集合として定義される。
自分の解法
各マスを走査し、1が見つかったらそのマスを起点にグラフ探索を行い、島の面積を計算します。グラフ探索では連結成分を取り出せれば十分なので、走査の順番は特に気にしません。DFSでもBFSでも何でも解けますが、いずれにせよパフォーマンスを考えて、反復法で実装します。
step1
O(n*m)O(n*m)step2
step1からの変更点
責務の分離(リファクタリング):
step1: 島の面積を計算するロジック(whileループ)が、グリッド全体を走査するforループの中に直接ネストされていました。これにより、maxAreaOfIsland関数が長大で複雑になっていました。step2: 島の面積を計算するロジックを、独立したヘルパー関数_calculate_area_bfsとして切り出しました。これにより、maxAreaOfIsland関数は「グリッドを走査し、新しい島を見つけたら面積計算を依頼する」という単一の責務に集中でき、コードの可読性と保守性が向上しました。探索アルゴリズムの変更:
step1:listをスタックとして利用し、pop()で要素を取り出す深さ優先探索(DFS)もどきを反復処理で実装していました。step2:collections.dequeをキューとして利用し、popleft()で要素を取り出す幅優先探索(BFS)を実装しています。アルゴリズムとしては何でもいい(連結成分さえ取り出せればOK)ですが、BFSの方が有名で、読み手が理解しやすいと考えました。ヘルパー関数の追加:
step1: 座標がグリッド範囲内かを確認するis_validのみでした。step2:is_validに加え、セルが陸地かどうかを判定するis_island関数を追加し、条件分岐の意図をより明確にしました。エッジケースの処理:
step1:if height<=0:でValueErrorを送出していました。step2:if not grid or not grid[0]:という、よりPythonicで一般的な方法で空のグリッドをチェックし、0を返しています。コードの可読性向上:
step1:i,jというループ変数が、外側のforループと内側のwhileループで再利用されており、混乱を招く可能性がありました。step2: 変数名をrow,columnのように、より意味の分かりやすいものに変更しました。また、方向を示すリストをDIRECTIONSという定数として定義し、マジックナンバーを排除しました。step3
step4 (FB)
別解・模範解答
gridに対してinplaceな実装をしてvisited_cellsを管理すれば空間計算量はO(1)になりますが、それは問題の要件依存だと思いました。step2_inplace.pyではそのような実装をしています。O(n*m)O(1)次に解く問題の予告