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

Build example games #22

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

Build example games #22

wants to merge 4 commits into from

Conversation

maxfierrog
Copy link
Member

No description provided.


/* TREE GAME UTILITY IMPLEMENTATIONS */

impl SimpleSum<2> for TreeExampleGame<'_> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Since all of the games here are simple utility, let's add an implementation like this one to all of the games in simple_utility.

This will allow us to call these games' code from solvers expecting a SimpleSum<2> game.

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, this might look like doing basically the exact same thing for all of the simple_sum games, so I recommend factoring out the match statement to minimize repeated code (as opposed to copying and pasting the impl block and changing the name 6 times).

src/game/mock/example.rs Outdated Show resolved Hide resolved

/* DEFINITIONS */

/// TODO
Copy link
Member Author

Choose a reason for hiding this comment

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

Add docstrings to written games with useful information about what they can be used to test (in other words, maybe a list of useful characteristics that the example game has).

src/game/mock/example.rs Outdated Show resolved Hide resolved
store.append(&mut nodes);
let store = &store[length..];

let game = builder::SessionBuilder::new(&TREE_GAME_NAME)
Copy link
Member Author

Choose a reason for hiding this comment

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

Great job making these nice and big, but let's try to not repeat the internal structures of the games among these standard tests. I haven't checked the other ones (as they do not render properly due to the naming issue), but at least this one seems to have nearly identical structure to the simple-utility general-sum cyclic example (line 295).

This is in order to get as much mileage out of these tests as we can. I recommend installing graphviz with brew install graphviz so that cargo test produces renders that you can compare to check they're "different enough."

@maxfierrog maxfierrog added feature New feature or request better engineering Non-essential refactoring, cleaning up code, etc. and removed feature New feature or request labels Apr 25, 2024
@BnjmnZmmrmn BnjmnZmmrmn added feature New feature or request stale No updates within last month labels Sep 18, 2024
@BnjmnZmmrmn
Copy link

BnjmnZmmrmn commented Sep 18, 2024

found this pr again lol
working on patch set to address merge conflicts + refactor contents
@maxfierrog any reason for removing feature label?

@BnjmnZmmrmn BnjmnZmmrmn mentioned this pull request Sep 19, 2024
@BnjmnZmmrmn
Copy link

i do not like how old me structured these commits; please see #37 before considering merge

@BnjmnZmmrmn
Copy link

nevermind, rebasing remote branch is hard. either way i want to refactor before merging

@maxfierrog
Copy link
Member Author

@maxfierrog any reason for removing feature label?

Nothing serious, I just thought this wasn't a "feature" per se. But sometimes, when I wake up on the other side of the bed, I do think it is a feature. I don't know, thinking about this philosophically ensnares me. Feel free to keep or remove it!

nevermind, rebasing remote branch is hard. either way i want to refactor before merging

Approved changes in refactor PR -- thanks! Feel free to request another review on this PR when you think it's GTG.

@BnjmnZmmrmn
Copy link

sg will do, thank u

@BnjmnZmmrmn
Copy link

todos to self
games todo are general/general/[acyclic, cyclic] and general/zero/[tree, acyclic, cyclic]
make sure testing is rigorous
add pretty comments

@BnjmnZmmrmn BnjmnZmmrmn removed the stale No updates within last month label Sep 30, 2024
@BnjmnZmmrmn BnjmnZmmrmn added this to the v1.0.0 release milestone Sep 30, 2024
diff:
+ src/game/mock/example/
	   !	     + general_utility/
	 /\_/\  (		    + general_sum/ + Tree - Ayclic - Cyclic
        ( ^.^ ) _)		    + zero_sum/    - Tree - Ayclic - Cyclic
	  \ /  (     + simple_utility/
        ( | | )			    + general_sum/ + Tree + Ayclic + Cyclic
       (__d b__)                    + zero_sum/    + Tree + Ayclic + Cyclic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
better engineering Non-essential refactoring, cleaning up code, etc. feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants