-
Notifications
You must be signed in to change notification settings - Fork 1
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
Updating notebooks #33
Conversation
Reviewer's Guide by SourceryThis pull request updates the README.md to provide direct links to the rendered HTML versions of the notebooks on ReadTheDocs and modifies the Sphinx configuration to improve navigation and table of contents settings. Additionally, it includes significant changes to the Sampler classes in the sampling module, updates to the Puzzle class, and enhancements to the unit tests. File-Level Changes
Tips
|
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.
Hey @Xmaster6y - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 5 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@sourcery-ai review |
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.
Hey @Xmaster6y - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 12 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
"widgets": { | ||
"application/vnd.jupyter.widget-state+json": { | ||
"03c1f0e5da5f48c799bc4e7e2229b2f9": { | ||
"model_module": "@jupyter-widgets/base", |
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.
suggestion: Consider consolidating widget state definitions.
There are multiple widget state definitions that appear to be very similar. Consider consolidating these definitions to reduce redundancy and improve maintainability.
closes #21
Summary by Sourcery
This pull request refactors the
Sampler
class and its subclasses to handle multiple boards, introduces new methods for evaluating multiple puzzles, and updates the documentation and tests accordingly. Additionally, a new notebook for running models on GPU has been added.evaluate_multiple
method in thePuzzle
class.board_move_generator
method in thePuzzle
class to yield board and move pairs.Sampler
class methods to handle multiple boards instead of a single board.RandomSampler
,ModelSampler
, andPolicySampler
to work with iterable boards.Puzzle
class to handle cases where themes or opening tags areNone
.Puzzle
class with a__len__
method to return the number of moves.README.md
to include new feature links and Colab badges.README.md
to point to the latest ReadTheDocs pages.Puzzle
class to verify puzzle creation and usage.Sampler
class methods.BatchedPolicySampler
and updated tests to usePolicySampler
instead.