733. Flood Fill#76
Conversation
| image_copy = copy.deepcopy(image) | ||
|
|
||
| num_rows = len(image_copy) | ||
| num_cols = len(image_copy[0]) |
There was a problem hiding this comment.
image が長さ0である場合も考慮して良いと思いました。
引数sr,sc が配列外参照している場合も考慮して点からも、こちらの場合を考慮する方が粒度が揃う(あり得る配列外参照を網羅できる)ので。
| return False | ||
| if col < 0 or num_cols <= col: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
配列外参照していないかどうかの判定であり、比較的すぐわかりやすい判定の部類だと思うので、early return などせず単に
return 0 <= row < num_rows and 0 <= col < num_colとしてしまっても差し障りないとも思います。
|
|
||
| coordinates = [(sr, sc)] | ||
| visited = set() | ||
| while coordinates: |
There was a problem hiding this comment.
提案というより質問なのですが、こちらのコードのように2つのlist を用いてBFSを実装するのはどのような理由からでしょうか?
こちらの練習会のコードではよく見る実装ですが、改めて考えると、単にcollections.deque を用いる実装でなくこちらの実装を選ぶ理由があまり明らかではないと思いました。
There was a problem hiding this comment.
BFS を deque でというのはこの話ですかね。
https://docs.google.com/document/d/11HV35ADPo9QxJOpJQ24FcZvtvioli770WWdZZDaLOfg/edit?tab=t.0#heading=h.ho7q4rvwsa1g
全体を見ないと整合性があるか分からないというのを私はわりと避けてます。あとで、誰かが書き換えて動かなくなるのが常なので。
There was a problem hiding this comment.
想定していた実装はリンク先で扱われている実装とは異なっているように思います.
具体的には以下のような実装を想定していました.レベルごとでなく単にFIFOで処理すればよいのでは?という程度の意見です.
import copy
import collections
class Solution:
def floodFill(self, image: List[List[int]], sr: int, sc: int, color: int) -> List[List[int]]:
# (前略)
original_color = image_copy[sr][sc]
coordinates = collections.deque([(sr, sc)]) # deque へ変更
visited = set()
while coordinates:
r, c = coordinates.popleft() # r, c を deque 先頭から取り出すように変更
visited.add((r, c))
image_copy[r][c] = color
for dr, dc in [(1, 0), (0, 1), (-1, 0), (0, -1)]:
new_r = r + dr
new_c = c + dc
if not is_in_image(new_r, new_c):
continue
if (new_r, new_c) in visited:
continue
if image_copy[new_r][new_c] != original_color:
continue
coordinates.append((new_r, new_c)) # deque 末尾へ直接追加するように変更
return image_copyThere was a problem hiding this comment.
これのことですかね?
There was a problem hiding this comment.
はい,頂いたリンク先のnodchipさんのコメントの以下の部分;
list を 2 つ使って、片方から展開したノードをもう片方に入れるやり方や、 deque の中に要素とレベルの tuple を入れるやり方のほうが良く見かけるかもしれません。
の「deque の中に要素とレベルの tuple を入れるやり方」と僕からお送りした実装はほとんど等価で,deque に要素のみを入れるやり方です.
質問したときに考えていたことを話すと,
- 今回実装したい手続きはレベルごとに処理が変わるようなものでないので,明示的にlist を2つ作ってレベルごとに分けて要素を持つことの必要性がすぐには分からなかった
- BFS そのものの手続きを視覚的に理解するとレベルごとに展開するという流れで実装することも理解できるが,レベルを明示的に扱わずにqueue (deque)を用いて要素をFIFOで処理することでBFS を実装するほうが,自分の知っている範囲では,よく見るものである cf. BFSのwikipedia, C++のライブラリ
ぐらいのことを考えていました.2点目の背景があったので1点目が気になったという構図です.
There was a problem hiding this comment.
2点目に関して、自分もよく見かける実装は一つのdequeを引き回す形ですね。
ただ、よく見かける=わかりやすいとは限らないと思っています。特にライブラリにおいては、わかりやすいよりも早さやメモリ効率が重視されそうです。
自分は、二つの配列を使ってレベルごとにbfsをする方法がわかりやすいと思ったのでそちらを採用しています。
そして、1点目に関連するのですが、自分の書くコードに一貫性を持たせるために、今回はレベルごとに処理が変わるわけでなくても、レベルごとに配列を用意する方法にしました
There was a problem hiding this comment.
分かりやすさと(今回のコードに限らない)一貫性の2点からレベルごとに配列を用意するようにされたのですね。
ライブラリ作成時における複数指標のトレードオフへの意識も含めて、おっしゃる通りだと思います。
ご回答ありがとうございました、自分でもやり取りの中で色々整理できました🙇 (長くなってすみません)
|
|
||
| original_color = image_copy[sr][sc] | ||
|
|
||
| coordinates = [(sr, sc)] |
There was a problem hiding this comment.
"floodFill操作により色をcolorへと変更するピクセルの" インデックス(座標)なので、"~"の部分の意味合いを変数名に入れても良いと思いました。
There was a problem hiding this comment.
coordinates_to_fillなどですかね。
今回はbfsで引き回してるだけで、bfsではfrontierとつけることもあることから
あまり命名に意味を持たせずに短い変数にしてました。
https://leetcode.com/problems/flood-fill/description/