-
-
Notifications
You must be signed in to change notification settings - Fork 360
Added flood fill in Coconut #743
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
Changes from 2 commits
a18e7cc
7d2739b
5da3ae8
d21dacd
aa08db0
a4dbc2a
9dcc06f
627aed9
fee7e83
85f8e9a
18944ea
f1aa057
7bc0e22
91d5625
5a6ebd0
153317d
0ca5ac6
7ada7da
c72caaf
70822e9
9d0a5bb
2793edf
8426216
e92bcb7
e631b57
02dd353
f9a6eda
b49f034
9d7349a
5be74ec
1949a10
a5c0f6b
db00314
f312994
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
from collections import deque | ||
import numpy as np | ||
|
||
|
||
data Point(x, y): | ||
def __add__(self, other is Point) = Point(self.x + other.x, self.y + other.y) | ||
|
||
|
||
# This function is necessary, because negative indices wrap around the | ||
# array in Coconut. | ||
def inbounds(canvas_shape, location is Point) = | ||
min(location) >= 0 and location.x < canvas_shape[0] and location.y < canvas_shape[1] | ||
|
||
|
||
def colour(canvas, location is Point, old_value, new_value): | ||
if not inbounds(canvas.shape, location): | ||
return | ||
|
||
if canvas[location] != old_value: | ||
return | ||
else: | ||
canvas[location] = new_value | ||
|
||
|
||
def find_neighbours(canvas, location is Point, old_value, new_value): | ||
possible_neighbours = (Point(0, 1), Point(1, 0), Point(0, -1), Point(-1, 0)) |> map$(location.__add__) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PEP8 recommends a maximum line length of 79. And while I think 79 is a very low for line length, this line in particular is a bit long. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm using a 90-ish line length (following Raymond Hettinger's advice), but this is indeed quite a bit too long. It probably needs the |
||
|
||
for neighbour in possible_neighbours: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this be a |
||
if inbounds(canvas.shape, neighbour) and canvas[neighbour] == old_value: | ||
yield neighbour | ||
|
||
|
||
def stack_fill(canvas, location is Point, old_value, new_value): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll agree with you that it's better to do stack work with imperative code. Don't try to squeeze functional code into this. |
||
if new_value == old_value: | ||
return | ||
|
||
stack = [location] | ||
|
||
while stack: | ||
current_location = stack.pop() | ||
colour(canvas, current_location, old_value, new_value) | ||
for neighbour in find_neighbours(canvas, current_location, old_value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe use stack.extend to add all new neighbours at once There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this one, I think it's a good idea |
||
new_value): | ||
stack.append(neighbour) | ||
|
||
|
||
def queue_fill(canvas, location is Point, old_value, new_value): | ||
if new_value == old_value: | ||
return | ||
|
||
queue = deque() | ||
queue.append(location) | ||
|
||
colour(canvas, location, old_value, new_value) | ||
|
||
while queue: | ||
current_location = queue.popleft() | ||
for neighbour in find_neighbours(canvas, current_location, old_value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same thing as above, you could do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I see how to do that right away. I probably need to work on it a bit more, but that seems out of my capabilities for now. |
||
new_value): | ||
queue.append(neighbour) | ||
colour(canvas, neighbour, old_value, new_value) | ||
|
||
|
||
def recursive_fill(canvas, location is Point, old_value, new_value): | ||
if new_value == old_value: | ||
return | ||
colour(canvas, location, old_value, new_value) | ||
|
||
for neighbour in find_neighbours(canvas, location, old_value, new_value): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a perfect candidate for a
The current implementation works of course, but it seemed like you wanted to have more functionalness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hum... something like that? find_neighbours(canvas, location, old_value, new_value) |> map$(recursive_fill$(canvas, ?, old_value, new_value) Probably, yes. I am not sure it won't be too confusing, though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely a little too long, splitting it up in About the confusion bit. I think that maps are as intuitive as for-loops. It's just that a lot of people only know for-loops, so maps seem weird to them. I may be slightly (very) biased here, as I have some experience with functional programming, so keep that in mind. |
||
recursive_fill(canvas, neighbour, old_value, new_value) | ||
|
||
|
||
if __name__ == '__main__': | ||
# Testing setup | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe try to make the tests similair to the Julia tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean "similar to the Julia tests"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disregard the thing I said. I meant to say that you should probably use the same test as in the Julia implementation, namely:
But I just realize that you are already doing so. It's just that Julia indexes it's arrays starting at 1. Got confused there. |
||
from collections import namedtuple | ||
|
||
TestResults = namedtuple('TestResults', 'passes failures') | ||
pass_count = failure_count = 0 | ||
|
||
grid = np.zeros((5, 5)) | ||
grid[2,:] = 1 | ||
solution_grid = np.zeros((5, 5)) | ||
solution_grid[:3,] = 1 | ||
|
||
starting_location = Point(0, 0) | ||
|
||
|
||
# The following is manual unit testing of the function | ||
recursive_fill(grid, starting_location, 0, 1) | ||
try: | ||
assert (grid == solution_grid).all() | ||
except AssertionError: | ||
print('F', end='') | ||
failure_count += 1 | ||
else: | ||
print('.', end='') | ||
pass_count += 1 | ||
|
||
# Resetting the grid, if everything went well. | ||
grid[:2,] = 0 | ||
|
||
stack_fill(grid, starting_location, 0, 1) | ||
try: | ||
assert (grid == solution_grid).all() | ||
except AssertionError: | ||
print('F', end='') | ||
failure_count += 1 | ||
else: | ||
print('.', end='') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it to look like how |
||
pass_count += 1 | ||
|
||
grid[:2,] = 0 | ||
|
||
queue_fill(grid, starting_location, 0, 1) | ||
try: | ||
assert (grid == solution_grid).all() | ||
except AssertionError: | ||
print('F', end='') | ||
failure_count += 1 | ||
else: | ||
print('.', end='') | ||
pass_count += 1 | ||
|
||
print('') | ||
print(TestResults(pass_count, failure_count)) | ||
|
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.
This function is no longer used in the Julia implementation for it's redundancy.
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.
I need to rework the code to remove that function, indeed