Skip to content
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

Cyclic and puzzle solvers #18

Draft
wants to merge 19 commits into
base: dev
Choose a base branch
from
Draft

Cyclic and puzzle solvers #18

wants to merge 19 commits into from

Conversation

maxfierrog
Copy link
Member

No description provided.

@maxfierrog maxfierrog added the feature New feature or request label Apr 18, 2024
@maxfierrog maxfierrog marked this pull request as ready for review April 18, 2024 22:04
@maxfierrog maxfierrog closed this Apr 18, 2024
@maxfierrog maxfierrog reopened this Apr 18, 2024
src/game/mod.rs Outdated

impl<G> SimpleSum<2> for G
where
G: ClassicGame + Extensive<2>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change ClassicGame to require Extensive<2> so that this line only reads G: ClassicGame.

src/solver/algorithm/strong/cyclic.rs Show resolved Hide resolved
}

#[cfg(test)]
mod tests {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you're using a bespoke GameGraph here -- please replace this with the existing game::mock::Session implementation (see game::mock::builder for how to make one).

This is probably from back when we had not finished it, so apologies for the delays on that.

Coordinate with @BnjmnZmmrmn if you would like him to make bigger test cases for you.

@@ -29,6 +29,7 @@ pub mod record {
pub mod mur;
pub mod sur;
pub mod rem;
pub mod surcc;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job doing this.

The record implementations are getting out of hand in terms of repeated code, but this is a good solution for now.

Let's start thinking about how we can refactor the record module in the future.

src/solver/algorithm/strong/cyclic.rs Show resolved Hide resolved
src/solver/algorithm/strong/cyclic.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/cyclic.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/cyclic.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/cyclic.rs Outdated Show resolved Hide resolved
@maxfierrog maxfierrog changed the title Puzzle solver and record implementations Cyclic solver and record implementations Apr 18, 2024
@maxfierrog maxfierrog changed the title Cyclic solver and record implementations Cyclic solver implementation Apr 18, 2024
@maxfierrog maxfierrog changed the title Cyclic solver implementation Cyclic and puzzle solvers Apr 18, 2024
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
src/solver/algorithm/strong/puzzle.rs Outdated Show resolved Hide resolved
Ok(())
}

fn basic_loopy_solver<G, D>(game: &G, db: &mut D) -> Result<()>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please refactor this if you get the chance, it is super long.

src/game/mod.rs Outdated
pub trait ClassicGame {
pub trait ClassicGame
where
Self: SimpleSum<2>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be where Self: Extensive<2>

src/game/mod.rs Outdated
pub trait ClassicPuzzle {
pub trait ClassicPuzzle
where
Self: SimpleSum<1>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Self: Extensive<1>

use crate::model::SimpleUtility;
use crate::model::{State, Turn};
use anyhow::Result;
use std::collections::{HashMap, VecDeque};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test imports should also follow ordering (external imports, stdlib, local imports)

/// its way up the game tree in reverse. Assigns a remoteness and simple
/// utiliity to every winning and losing position. Draws (positions where
/// winning is impossible, but it is possible to play forever without losing)
/// not assigned a remoteness. This implementation uses the SURCC record to
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draws are* not

SimpleUtility::TIE => Err(SolverViolation {
name: "PuzzleSolver".to_string(),
hint: format!(
"Primitive end position cannot have utility TIE
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you make multi-line strings, use a backslash after a space \ to truncate them (without exceeding the 80-column limit) and ensure they are grammatically correct:

hint: format!(
  "Primitive end position cannot have utility TIE \
  for a puzzle."
),

Ok(())
}

/// Set up the initial frontiers and primitive position database entries
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing period, grammatically off -- it is standard to write docstrings with the function as the object of the sentence (grammatically speaking):

/// [the function] Sets up ... database entries.

The words in brackets are there to show how it works out grammatically; it's not that they should also be there.

Some(SimpleUtility::WIN) => winning_frontier.push_back(curr_state),
Some(SimpleUtility::TIE) => tying_frontier.push_back(curr_state),
Some(SimpleUtility::LOSE) => losing_frontier.push_back(curr_state),
_ => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using _ in match statements, opt instead to be exhaustive (for the same reason as I previously explained).

Some(SimpleUtility::TIE) => tying_frontier.push_back(curr_state),
Some(SimpleUtility::LOSE) => losing_frontier.push_back(curr_state),
_ => {
panic!["Utility for primitive ending position found to be draw"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use parentheses for non-collection macro invocations

}

fn discover_end_states<G, D>(db: &mut D, game: &G) -> Vec<State>
/// Updates the database record for a puzzle with given simple utility,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing period

@@ -80,4 +82,7 @@ pub enum RecordType {
SUR(PlayerCount),
/// Remoteness record (no utilities).
REM,
/// Simple Utility Remoteness with Child Counts records for a specific
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing period

@maxfierrog maxfierrog marked this pull request as draft April 25, 2024 05:27
@BnjmnZmmrmn BnjmnZmmrmn added this to the v1.0.0 release milestone Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants