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 rps automata #42

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

whonut
Copy link

@whonut whonut commented Jun 3, 2020

Addresses #26.

Apologies if I should've asked before cracking on with this. There are basically two main additions at present:

  1. A new rule implementing the rock-paper-scissors automota. The implementation relies on storing a copy of the board for each state in a 3D binary/boolean array. The reason I did it this way rather than using a single integer array with different numbers for each state is that it seemed the obvious way to allow use of existing Lifeforms. It occurs to me as I write this that it would be fairly easily to do this using a single 2D integer array too. I look forward to your thoughts on this.

  2. A MultiStateBoard which keeps track of automata with multiple states and allows existing lifeforms to be added in a given state.

I haven't gotten animation working at present but I thought I'd open a PR before I start hacking at existing code! I think my additions are in a good enough state to have a discussion at this point.

Addresses ljvmiranda921#26.

The current implemenation maintains separate 2D board matrices
for each state to allow the use of existing lifeforms.
This board allows for the use of multistate automata.
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #42 into master will decrease coverage by 11.02%.
The diff coverage is 14.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #42       +/-   ##
===========================================
- Coverage   97.76%   86.73%   -11.03%     
===========================================
  Files          12       12               
  Lines         268      309       +41     
===========================================
+ Hits          262      268        +6     
- Misses          6       41       +35     
Impacted Files Coverage Δ
seagull/rules.py 73.68% <9.09%> (-26.32%) ⬇️
seagull/board.py 57.62% <16.66%> (-42.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bad223...ce58213. Read the comment docs.

@ljvmiranda921
Copy link
Owner

Hi @whonut , thanks for this! Let me check your code this weekend! Hope you had fun working on it!

@ljvmiranda921 ljvmiranda921 self-requested a review June 6, 2020 03:08
@ljvmiranda921 ljvmiranda921 added documentation Update package documentation enhancement New feature or request labels Jun 6, 2020
@ljvmiranda921
Copy link
Owner

Apologies if I should've asked before cracking on with this.

No worries!

The implementation relies on storing a copy of the board for each state in a 3D binary/boolean array. The reason I did it this way rather than using a single integer array with different numbers for each state is that it seemed the obvious way to allow use of existing Lifeforms.

This is actually a better solution that what I was thinking. I realized that it might be difficult to reuse the Lifeforms if I create a board that uses different integers instead of the binary one. Would you mind elaborating on a high-level how you're implementing the logic? I assume that (forgive, I haven't looked at the code yet):

  • The 3D states will correspond to rock, paper, and scissors.
  • Only one "slot" should be filled at a given time, so if the rock slot was filled, then it's a rock.

Let me look at the code in a few minutes and add my thoughts!

Copy link
Owner

@ljvmiranda921 ljvmiranda921 left a comment

Choose a reason for hiding this comment

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

Looks pretty good! I think the bulk of the remaining tasks would be documentation and testing. I'd suggest adding:

class MultiStateBoard:
"""
Represents a multistate environment where the lifeforms can grow and
evolve
Copy link
Owner

Choose a reason for hiding this comment

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

Once we've settled on the setup, I'd highly recommend elaborating further in the docstring how the MultiStateBoard "works"! 😄

Copy link
Author

Choose a reason for hiding this comment

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

Of course :)

seagull/rules.py Show resolved Hide resolved
Renames MultiStateBoard.states to state for consistency
with Board.
The fix ensures that, when adding lifeforms, each cell is in exactly
one state at a time i.e. only one of the entries in the state tensor
corresponding to that cell is set to True at any one time.
board.py now conforms with black style guide.
Fixed a bug with rps_life_rule with neighbour counting
caused by the array holding neighbour counts inheriting the
dtype bool from the state array.
@whonut
Copy link
Author

whonut commented Jun 6, 2020

So I've squashed some bugs so it actually works now! 😳

The major thing that needs doing now is some sort of modification of Simulator to get it working with MultiStateBoard. I'd be inclined to reimplement Board as a special case of it and rework simulation to work with MultiStateBoard, but that's just me. We could split off the 'flattening' currently done in MultiStateBoard.view and use it in animate too. Let me know what you think.

@ljvmiranda921
Copy link
Owner

I'd be inclined to reimplement Board as a special case of it and rework simulation to work with MultiStateBoard, but that's just me.

Let's treat this as another Issue or PR! I'm fine keeping them separate for now. Perhaps you can create a base class, BaseBoard that both the Board and MultiStateBoard can inherit for the Simulator? (or let MultiStateBoard inherit from Board with your modifications).

Either way, I'm fine even with having a MultiStateSimulator and just putting proper error messages when one is used for the other. I try to be careful of generalizing too early 👍

Sorry won't be able to check this until Sunday (or the next weekend, not this week)! Will be on vacation leave for a bit to recharge! Will find a timeto read through this and add my comments!

@whonut
Copy link
Author

whonut commented Jun 9, 2020

Either way, I'm fine even with having a MultiStateSimulator and just putting proper error messages when one is used for the other. I try to be careful of generalizing too early 👍

Fair enough, I'm pretty new at this!

Sorry won't be able to check this until Sunday (or the next weekend, not this week)! Will be on vacation leave for a bit to recharge! Will find a timeto read through this and add my comments!

I look forward to it!

Copy link
Owner

@ljvmiranda921 ljvmiranda921 left a comment

Choose a reason for hiding this comment

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

Looks good! The only thing remaining are the tests so that the code coverage checks will pass 👍 let me know if you need some guidance in doing them, @whonut !

@ljvmiranda921
Copy link
Owner

Hi @whonut , any updates on this? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Update package documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants