-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Conversation
* Adding flood_fill.c * Updating flood_fill.md * adding C's queue_fill to flood_fill.md
* Add code and image for huffman * Fix a missing line and a few typo
([0,radius], [0, radius]) is in the upper RIGHT quadrant, as mentioned in the explanations for Monte Carlo.
…ists#749) * Fixed infinite loop issue in "euclidean_example.py" * Changed line numbers in "euclidean_algorithm.md" for python
* fix both error of issue 739 * Update contents/huffman_encoding/code/python/huffman.py Co-authored-by: James Schloss <jrs.schloss@gmail.com>
* add floodfill in python * add name to contributors * small correction * remove unnecessary argument to color() * switch to using numpy + small corrections * improved unit testing * Apply suggestions from code review Co-authored-by: James Schloss <jrs.schloss@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
…-lisp Added Cooley-Tukey in Common Lisp
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.
The .md file still needs some work, but the code compiles runs and succeeds.
It also feels like the Coconut features can be used slightly more.
In conclusion: I've learned a new dialect today and it's pretty neat!
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__) | ||
|
||
for neighbour in possible_neighbours: |
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.
Can't this be a filter
, those are usually available in most functional languages
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): |
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
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a perfect candidate for a map
and a lambda-function:
map (x -> recursive_fill(canvas, x, old_value, new_value)) (find_neighbours etc...)
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely a little too long, splitting it up in neighbours = find_neighbours(...)
and neighbours |> map$(...)
is probably a good idea.
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
For this one, I think it's a good idea
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 comment
The 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.
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
same thing as above, you could do map
for colour and queue.extend
to add.
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 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.
|
||
|
||
if __name__ == '__main__': | ||
# Testing setup |
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.
Maybe try to make the tests similair to the Julia tests.
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.
What do you mean "similar to the Julia tests"?
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.
Disregard the thing I said. I meant to say that you should probably use the same test as in the Julia implementation, namely:
grid = zeros(5,5)
grid[3,:] .= 1
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.
print('F', end='') | ||
failure_count += 1 | ||
else: | ||
print('.', end='') |
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.
The .
is a bit vague, maybe print something slightly more obvious S
for succes, or maybe just full words: Success and Failure.
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 made it to look like how unittest
runs tests, so that's a revertible design choice
|
||
|
||
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 comment
The 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 comment
The 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 map
on the next line, along with the possible filter
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.
So, I know it needs rework, but some points you raised are, I think, not really valid.
I'm glad to see more people work with Coconut, though :D
|
||
|
||
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 comment
The 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 map
on the next line, along with the possible filter
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 comment
The 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
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): |
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
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 comment
The reason will be displayed to describe this comment to others. Learn more.
For this one, I think it's a good idea
|
||
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 comment
The 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.
|
||
|
||
if __name__ == '__main__': | ||
# Testing setup |
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.
What do you mean "similar to the Julia tests"?
print('F', end='') | ||
failure_count += 1 | ||
else: | ||
print('.', end='') |
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 made it to look like how unittest
runs tests, so that's a revertible design choice
Sometimes I am just wrong, so feel free to call me out. It's only my first time touching the language after all. |
* Added domain coloring in Python * Some cleanup
Closing to redo because I did something stupid. |
Here is the new Coconut code for the flood fill chapter.
I don't see how to make the code more functional, sadly, as it's a really iterative method.
There might be a merge conflict with the C code for now, but in the meantime, this PR is waiting for review.