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

Add Game of Life exercise #2281

Merged
merged 12 commits into from
Apr 5, 2024
Merged

Add Game of Life exercise #2281

merged 12 commits into from
Apr 5, 2024

Conversation

Steffan153
Copy link
Contributor

https://forum.exercism.org/t/conways-game-of-life-exercise/5509

I don't think this is enough tests, but I'm not quite sure what to test. Should I add more random matrices, or should they be more specific cases?

@Steffan153 Steffan153 requested a review from a team as a code owner May 6, 2023 22:28
@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this May 6, 2023
@IsaacG IsaacG reopened this May 7, 2023
@IsaacG
Copy link
Member

IsaacG commented May 7, 2023

Do you want to have the number of turns be an input parameter?

@igbanam
Copy link

igbanam commented May 7, 2023

To test for efficiency, the problem statement could be "How many generations are there between populations X and Y?"

…though, this would mean we have a timeout on the test-runner

[1, 1, 0],
[0, 0, 0],
[1, 0, 0]
]
Copy link
Member

Choose a reason for hiding this comment

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

I like the choice of a matrix of ones and zeros to represent the input and output. I would imagine that most languages can use that in their implementations with very little fuss.

"matrix": [
[1, 1, 0],
[0, 0, 0],
[1, 0, 0]
Copy link
Member

Choose a reason for hiding this comment

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

I like the choice of a matrix of ones and zeros to represent the input and output. I would imagine that most languages can use that in their implementations with very little fuss.

[1, 1, 0, 1, 0, 0, 0, 1],
[1, 0, 0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, 0, 0, 1, 1]
]
Copy link
Member

Choose a reason for hiding this comment

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

In order to fully specify the basic rules of the cellular automata, I think we should have at least the following tests:

  1. Underpopulation: live cell that dies due to having no live neighbors
  2. Underpopulation: live cell that dies due to having only one live neighbor.
  3. Survival: A live cell that survives to the next generation due to having two live neighbors.
  4. Survival: A live cell that survives to the next generation due to having three live neighbors.
  5. Overcrowding: A live cell that dies due to having four live neighbors.
  6. Reproduction: A dead cell with exactly three live neighbors becomes a live cell.

In each of these, there would only be one turn.

I would instinctively want to have individual tests for a live cell that dies due to overcrowding with five, six, seven, and eight live neighbors, but I don't know this problem well, so I can't tell if this would be overkill.

Ideally each of these tests would have the smallest possible matrix with the fewest possible changes.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a very good idea. I think it would be great to see a test for each count of neighbors. Maybe with an empty border so there's no literal edge cases.

Would it also make sense to have a grid which combines all those individual minimal unit tests into a single test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it also make sense to have a grid which combines all those individual minimal unit tests into a single test?

That would be a puzzle:

Alive neighbors 0 1 2 3 4 5 6 7 8
Cases 1 8 28 56 70 56 28 8 1

(Multiply by 2 to account for the focus being alive/dead.)

Also, it is impossible to have test cases that only check certain numbers of alive neighbors, for all numbers. (But is is very much possible for some.)

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't have to be perfect.

# Zero
000
000
000

# One
100
000
000

# Two
110
000
000

# Three 
101
000
001

# Etc

Copy link
Member

Choose a reason for hiding this comment

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

For combining, I meant literally having a larger matrix which has the minimal matrices copy pasted into non overlapping sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we don't need an Overcrowding test for every number, but the test I added for it naturally has different numbers of neighbors for each cell.

Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this test? "Random" data doesn't make for a good test. Maybe "larger board" or "bigger test"? If it's random, what is it actually testing?

{
"uuid": "ae86ea7d-bd07-4357-90b3-ac7d256bd5c5",
"description": "empty matrix",
"property": "gameOfLife",
Copy link
Member

Choose a reason for hiding this comment

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

In terms of exercise design, I'd like to have a different name for the property.

If the implementation of the exercise gets created via a generator we'd end up with both the top level namespace and the property under test being game of life. I'd like to start with names that are a reasonable default. People can always choose to override this in an individual track, but starting with a good default helps tremendously.

I'm trying to think of names that have to do with the flow of generations, or population... maybe tick (the generations tick by)?

Copy link
Member

Choose a reason for hiding this comment

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

tick, round, simulate, advance, next

Any or all of those sounds good to me

Copy link
Member

Choose a reason for hiding this comment

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

I like all of those too

@IsaacG
Copy link
Member

IsaacG commented May 7, 2023

To test for efficiency, the problem statement could be "How many generations are there between populations X and Y?"

…though, this would mean we have a timeout on the test-runner

I believe all test runners have time outs. It's part of the test runner infrastructure.

Another approach is asking how many cells are alive after N rounds. While it might be more "fun" or interesting to have different function signatures, I'm not sure it adds too much to the problem. It does provide a bit more room for flexibility in the approach; if I'm just counting cells or generations until a certain pattern, it would be easier to switch from a 2D array to a collection of points (which is more efficient for sparse data). If I'm doing a single tick, using a sparse collection is harder.

@Steffan153
Copy link
Contributor Author

Sorry I took so long! I've added a bunch more tests.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just a couple of tiny nits.

exercises/game-of-life/metadata.toml Show resolved Hide resolved
exercises/game-of-life/description.md Outdated Show resolved Hide resolved
@Steffan153
Copy link
Contributor Author

Does this need an icon btw?

@ErikSchierboom
Copy link
Member

Does this need an icon btw?

It does. We'll get it sorted.

exercises/game-of-life/introduction.md Outdated Show resolved Hide resolved
exercises/game-of-life/introduction.md Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Great work!

[1, 1, 0, 1, 0, 0, 0, 1],
[1, 0, 0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, 0, 0, 1, 1]
]
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this test? "Random" data doesn't make for a good test. Maybe "larger board" or "bigger test"? If it's random, what is it actually testing?

Copy link
Contributor

@MatthijsBlom MatthijsBlom left a comment

Choose a reason for hiding this comment

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

I can't find the right headspace right now to review this thoroughly, sorry.

@ErikSchierboom
Copy link
Member

Cc @exercism/reviewers

@ErikSchierboom
Copy link
Member

@glennj or @BethanyG, would one of you have time to review this?

exercises/game-of-life/canonical-data.json Outdated Show resolved Hide resolved
@kotp kotp merged commit 381bb40 into exercism:main Apr 5, 2024
7 checks passed
@ErikSchierboom
Copy link
Member

Thanks @Steffan153 for creating this PR! It's a wonderful addition,

@Steffan153 Steffan153 deleted the game-of-life branch April 5, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants