Hierarchy of classes Grid, SingleGrid, and MultiGrid

Describe the bug

Code snippet from class Grid

    def remove_agent(self, agent):
        """ Remove the agent from the grid and set its pos variable to None. """
        pos = agent.pos
        self._remove_agent(pos, agent)
        agent.pos = None

    def _remove_agent(self, pos, agent):
        """ Remove the agent from the given location. """
        x, y = pos
        self.grid[x][y] = None
        self.empties.append(pos)

As you see, Grid._remove_agent is clearly assuming a SingleGrid behavior. This is fine. However, it is also assuming that None is defined as an empty cell, which is not what Grid.is_cell_empty does:

    def is_cell_empty(self, pos):
        """ Returns a bool of the contents of a cell. """
        x, y = pos
        return True if self.grid[x][y] == self.default_val() else False

Since Grid.default_value == None, it seems ok so far. But I guess we need to make it consistent.

So I was thinking SingleGrid and Grid are really the same. By further looking at the class SingleGrid, I found these two methods in it:

    def position_agent(self, agent, x="random", y="random"):
        """ Position an agent on the grid.
        This is used when first placing agents! Use 'move_to_empty()'
        when you want agents to jump to an empty cell.
        Use 'swap_pos()' to swap agents positions.
        If x or y are positive, they are used, but if "random",
        we get a random position.
        Ensure this random position is not occupied (in Grid).

        """
        if x == "random" or y == "random":
            if len(self.empties) == 0:
                raise Exception("ERROR: Grid full")
            coords = agent.random.choice(self.empties)
        else:
            coords = (x, y)
        agent.pos = coords
        self._place_agent(coords, agent)

    def _place_agent(self, pos, agent):
        if self.is_cell_empty(pos):
            super()._place_agent(pos, agent)
        else:
            raise Exception("Cell not empty")

Where in class Grid they are

    def place_agent(self, agent, pos):
        """ Position an agent on the grid, and set its pos variable. """
        self._place_agent(pos, agent)
        agent.pos = pos

    def _place_agent(self, pos, agent):
        """ Place the agent at the correct location. """
        x, y = pos
        self.grid[x][y] = agent
        if pos in self.empties:
            self.empties.remove(pos)

So the default behavior of Grid.place_agent is replacing the original agent if a cell has one, otherwise place to an empty cell. The default behavior of SingleGrid.place_agent on the other hand binds the agent position when a cell is assigned an agent.

I think using position_agent as another method name just to do the enforcement can be a bit confusing. Plus, its signature does not match with place_agent, which can lead to weird bug. For example, I tried position_agent(agent=agent, pos=(x,y)) when I was implementing a model. My agent got placed to some random position because y == "random". To further improve this issue, we might want to allow users to fix one of the two coordinates and set the other one to "random".

I hope I am stating the issues clearly...I would work on it these two days. I never used CI tools like Travis before, and I guess the tests will fail once I do some changes to the overall hierarchy of classes. Should I change the test files at the same time? I am guessing my own test files won't be used any way and the test will fail. Is there any workaround for this?

projectmesa / mesa

Hierarchy of classes Grid, SingleGrid, and MultiGrid #623