-
Notifications
You must be signed in to change notification settings - Fork 859
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
Merge Grid, SingleGrid, and MultiGrid into one class #625
Conversation
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.
Great PR! However, as the changes would break every model with a Grid, we have to be clever on how to to the transition.
I think calls to SingleGrid and MultiGrid should call the appropriate version of Grid and issue a Depreciation warning. Same for some of the removed functions.
I added a bunch of code comments, sometimes they are specific to your PR, but some a more general remarks on the grid functions.
I also opened a topic on the mailing list to discuss how to proceed with you proposed changes
mesa/space.py
Outdated
|
||
def neighbors(self, pos, moore=True, radius=1, get_agents=False, include_empty=False): | ||
""" |
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.
Summary line and explanation of the method is missing
mesa/space.py
Outdated
self.empties.remove(pos) | ||
else: | ||
if replace: | ||
old_agent.pos = None |
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.
I think this will fail for multigrid? As old_agent would be a set. It is also not clear how "replace" should work for a multigrid. Should it replace a single agent? All agents? Or can't it be used with MultiGrid?
mesa/space.py
Outdated
"Cell already occupied by agent {}".format(old_agent.unique_id)) | ||
|
||
def _pick_random_position(self, agent, pos): | ||
if pos == ("random", "random") and self.exists_empty_cells(): |
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.
I think the function should also work with other sequences, e.g. ["random", "random"]
mesa/space.py
Outdated
else: | ||
empties = set() | ||
if pos[0] == "random": | ||
# We pick a random cell in a specified row |
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.
Comment and code got me confused. You don't pick a random cell here, you just make a set (why not list?) of empty cells and then randomly select one further down
mesa/space.py
Outdated
@@ -322,21 +273,26 @@ def remove_agent(self, agent): | |||
def _remove_agent(self, pos, agent): | |||
""" Remove the agent from the given location. """ |
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.
The agents pos attribute should also be set to None
Thanks a lot for the comments! I was rushing a bit to show the hierarchy. I was also a bit conservative due to backward compatibility problem like you said. I haven't really taken a good look at any other classes yet, so I am hesitant about merging |
btw, is there any motivation to name a network |
Really quick, so you get this info if you want to work on this on the weekend: I tested your branch and there are several severe bugs. I suggest adding class SingleGrid(Grid):
"""Depreciated class."""
def __init__(self, width, height, torus):
super().__init__(width, height, torus, multigrid=False)
class MultiGrid(Grid):
"""Depreciated class."""
def __init__(self, width, height, torus):
super().__init__(width, height, torus, multigrid=True) to your file and then run the tests to see a lot of errors (not just related to missing functions). I already made some changes. I wanted to PR my changes into your repo, but since you did not fork from projectmesa, I wasn't able to tell GitHub where to PR. So here is a gist of my file based on your changes. |
As noted already there are several bugs in this implementation. See this PR to your repo for which ones and how I solved them. I would suggest the following approach: Make sure the current tests work (by adding in deprecated functions and/or resolve bugs). Then add new tests based on the current implementation. Again, I really like your changes/new structure but its not ready for a merge yet. I would also like to see a return of the get_neighborhood function (including center). |
Thanks a million for the efforts! I know the current code was committed under rush (sorry for that) and I intended to work on it this week. |
Add deprecated classes and functions
I don't understand why I pass the tests locally but fail in Travis...I really don't want to push a lot to annoy you, but I am not sure why this happens.
In addition, the failure is
However, I have passed the local tests when I did the PR #622, and I haven't updated/downgraded the package in any way. Why would this error suddenly pops up? |
Exciting new commits! I will review your changes and give you more feedback in the next days. RE tests: Maybe try to clean your local |
@kumom this is a great PR. At first glance / skim, I have to say I agree with Corvince. If you aren't a part of the dev group -- can you join us there for the discussion? (Otherwise, we will report back here.) https://groups.google.com/forum/#!topic/projectmesa-dev/H7IUeol8iRM |
I am in the group already. Thanks a lot for the comments. |
After cleaning the cache, I passes all the tests (with the buggy code)...
|
Here is something that's up to a discussion: You have removed the I think the use cases for both functions are quite distinct and I would like to keep them separate. Also your function defaults would return a list of cell coordinates, which are not empty. I think that is kind of odd. But as I said this is just my personal opinion/expectation. Feel free to discuss with me |
Thats strange. When I run your changes locally the same tests fail as on travis |
@kumom Thanks for working on this -- my memory is that the split into single/multi-grid was a path-dependent artifact and not a well-thought-out design choice, so this is probably long overdue. I agree with @Corvince that it makes sense to have separate methods for getting I'm also seeing the same test failures on my end as Travis is showing. |
I am still debugging the issue with pytest, and I think I have figured out why all tests are passed - because mesa installed via pip was imported. However, after I ran
I ran this under a new folder cloned from projectmesa/mesa. Why would this happen? Am I doing something wrong with pytest? |
Pytest ist fine, but you still have to install Mesa. Instead of pip install Mesa, which pulls it from pypi, try to install it in the root dir (projectmesa/Mesa) with |
Thanks a lot! Problem solved already |
@Corvince At some point in developing the PropertyGrid in #1898 I also considered merging them. Instead of a binary Do you think ideally we should pursue merging these classes? If so, do you think it's worth it? The reasons I didn't where 1) backwards compatibility and 2) performance (some functions are faster if there can be only 1 agent anywhere or if there can be infinite somewhere).
|
I think that's a pretty solid idea and I think it would be possible to do this in a backwards compatible way. I would guess the devil will be in the details, for example for grid access do you return a single agent or a list of a single agent? The latter would be more consistent but possibly confusing (why return a list if there can only be one agent) |
This is some follow-up work for reasons explained in #623. I basically merge three classes (
Grid
,SingleGrid
,MultiGrid
) into one, and usemultigrid
as a boolean attribute to specify the behavior of the class. I haven't modified the test files, so it will for sure fail since I changed some method signatures. In addition, I noticeHexGrid
is a subtype ofGrid
, and this modification will cause some of its behaviors to fail potentially.What I intend to do next, if the current hierarchy looks good to you, is to change
multigrid
intogrid_type
. Allowed values ofgrid_type
would besingle
,multi
, orhex
, so thatHexGrid
would be "merged" toGrid
class as well. Just a side node, I adopt this style of code because I am somewhat influenced by the style of sklearn.I also had this question as I read the source code. The only reason I can think of is probably to avoid concurrency issues/code optimization done by compiler. Is there any good reason to write the code this way?