-
Notifications
You must be signed in to change notification settings - Fork 357
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
Remove PIL dependency in Sudoku Example #2467
Conversation
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.
Thanks! In addition to my suggestions below, the sudoku_solver.py
script also needs to be updated to work with the changed function arguments in helpers.py
.
Co-authored-by: Håkon Bakke Mørk <hakon.mork@nmbu.no>
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.
Thanks for the changes. I have a few more suggestions.
Co-authored-by: Håkon Bakke Mørk <hakon.mork@nmbu.no>
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.
Thanks!
@krachbumm3nte Could you pull master into this branch? Then the macOS test should pass (see #2515). |
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.
Many thanks for this PR. Other than a couple of formatting issues, I'm fine.
Co-authored-by: Jochen Martin Eppler <jougs@gmx.net>
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.
Thanks for the quick reaction. I think this looks much better now.
@krachbumm3nte Thanks a lot, I will merge this now. I noticed that many docstrings begin with lowercase letters, e.g., def get_puzzle(puzzle_index):
"""returns one of 8 Sudoku configuration to be solved. Our style is to begin docstrings with capital letters, e.g., here """Returns one ...". Could you at some point create another PR fixing this? |
As discussed in #2441, I have updated the visualization backend for the Sudoku example to rely fully on matplotlib and imageio.