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

Implement RasterLayer #75

Merged
merged 1 commit into from
Jul 10, 2022
Merged

Conversation

wang-boyu
Copy link
Member

@wang-boyu wang-boyu commented Jul 4, 2022

This PR is not yet ready, but I'd like to gather some feedbacks from you @rht @Corvince

The classes now look like this:

classDiagram
GeoBase <|-- GeoAgent 
GeoBase <|-- GeoSpace
GeoAgent <-- GeoSpace: contains
GeoBase <|-- RasterBase
RasterBase <-- GeoSpace: contains
RasterBase <|-- ImageLayer
RasterBase <|-- RasterLayer
Cell <-- RasterLayer: contains
Loading

The main pending works are:

  1. Visualization of RasterLayer

    Currently RasterLayer uses a portrayal method to map a Cell to a color, i.e., (r, g, b, a). But it would be better if the users can define the portrayal method to return a dict (similar to agent portrayal). And I'm not sure how this works with a range of colors. Example from NetLogo: scale-color .

  2. The get_min_cell and get_max_cell methods

    Instead of the current implementation, I was thinking to have something more generic such as

    get_cell(
       self,
       pos: Coordinate,
       by: str | List[str] | Callable[Cell, float],  # different from current implementation
       aggfunc: Callable[[List[Cell]], Cell],        # different from current implementation
       moore: bool,
       include_center: bool = False,
       radius: int = 1,
    )

    so that the users can pass min and max as the aggfunc parameter. In addition, users can also define their own by parameter to be any function instead of cell attributes. But this API seems a bit too complicated.

  3. The apply_raster method

    This is essentially the gis:apply-raster from NetLogo. But currently it works only if the input matrix has exactly the same dimension as the Raster Layer. Shall I make it more flexible so that the input matrix gets automatically resized to that of the Raster Layer?

Please feel free to suggest how to improve this PR. Thanks a lot!

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #75 (2622e87) into master (b3f9e8b) will decrease coverage by 7.58%.
The diff coverage is 70.76%.

❗ Current head 2622e87 differs from pull request most recent head 9962032. Consider uploading reports for the commit 9962032 to get more accurate results

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   85.67%   78.09%   -7.59%     
==========================================
  Files           5        7       +2     
  Lines         363      566     +203     
  Branches       57       99      +42     
==========================================
+ Hits          311      442     +131     
- Misses         37      108      +71     
- Partials       15       16       +1     
Impacted Files Coverage Δ
mesa_geo/visualization/modules/MapVisualization.py 87.17% <62.50%> (+3.39%) ⬆️
mesa_geo/geospace.py 84.84% <67.56%> (-3.42%) ⬇️
mesa_geo/geoagent.py 78.94% <70.00%> (+0.78%) ⬆️
mesa_geo/raster_layers.py 70.56% <70.56%> (ø)
mesa_geo/geo_base.py 76.92% <76.92%> (ø)
mesa_geo/__init__.py 100.00% <100.00%> (ø)

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 b3f9e8b...9962032. Read the comment docs.

@wang-boyu wang-boyu force-pushed the feat/raster-layer branch 2 times, most recently from 69f94f0 to 2622e87 Compare July 4, 2022 11:34
@rht
Copy link
Contributor

rht commented Jul 4, 2022

I think the 2 examples can be in separate PRs. Imagine the changelog for the release associated with this PR: "Implement RasterLayer and 2 examples", that would be putting too much in.

@wang-boyu
Copy link
Member Author

Sure. The examples are in a separate branch now: https://github.com/wang-boyu/mesa-geo/tree/examples

@wang-boyu wang-boyu changed the title Implement RasterLayer and add two examples Implement RasterLayer Jul 4, 2022
@rht
Copy link
Contributor

rht commented Jul 4, 2022

In addition, users can also define their own by parameter to be any function instead of cell attributes. But this API seems a bit too complicated.

This might be an opportunity for designing a powerful API like you'd see in Pandas DF col/row aggregation. E.g. min(self.space.get_neighboring_cells(pos, ...).apply(lambda x: x.color ** 2))

@Corvince
Copy link
Contributor

Corvince commented Jul 4, 2022

Looking forward to reviewing this! For me it would have been beneficial to have the examples in this pr to immediately see how it is being used, but I am also fine with a separate branch

@wang-boyu
Copy link
Member Author

Thanks! And sorry you'll have to check the examples from the separate branch now : )

@wang-boyu
Copy link
Member Author

This might be an opportunity for designing a powerful API like you'd see in Pandas DF col/row aggregation. E.g. min(self.space.get_neighboring_cells(pos, ...).apply(lambda x: x.color ** 2))

This is a good point! But I think I'll leave it to the next PR - to start with something like a CellGroupBy class that is in analogy to Pandas' GroupBy, due to the amount of work involved.

For this PR I'll remove the get_min_cell and get_max_cell methods. We can do these operations with min(self.space.get_neighboring_cells(pos, ...), key=lambda cell: cell.attribute) instead.

def __getitem__(self, index: Sequence[Coordinate]) -> List[Cell]:
...

def __getitem__(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the code in this method copied almost verbatim from mesa.space.Grid? If so, we should inform in the docstring about it. But there are potential new changes to it in projectmesa/mesa#815. Having to manually sync between them would not be ideal.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right! Initially I planned to make RasterLayer derived from Grid:

class RasterLayer(GeoBase, mesa.space.Grid):
    ...

But then I don't want it to be able to hold agents, since we want our users to add agents directly into GeoSpace.

I'm also aware of the new changes, but they are not yet released. I hope such changes won't happen too often in the future so that manual sync remains feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could define a common class in Mesa that has this method. Is __getitem__ the only one that is copied over so far?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid not. What's copied over so far:

__getitem__
__iter__
coord_iter
iter_neighborhood
get_neighborhood
iter_neighbors
get_neighbors  # copied and renamed to `get_neighboring_cells`
out_of_bounds  # not yet copied but could be useful
iter_cell_list_contents
get_cell_list_contents

Methods that are not copied over:

torus_adj
neighbor_iter
move_agent
place_agent
_place_agent
remove_agent
is_cell_empty
move_to_empty
find_empty
exists_empty_cells

Another difference is that Grid has self.grid: List[List[Agent | None]], whereas it is self.cells: List[List[Cell]] in RasterLayer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a lot of methods. We should definitely replace the copying into inheritance very soon. I'm fine with it for now though.

Copy link
Member Author

@wang-boyu wang-boyu Jul 7, 2022

Choose a reason for hiding this comment

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

That's indeed a lot of methods. Inheritance would make it much better, but I would not want to mess up the classes in Mesa either.

Edit: Sorry by saying "mess up" I meant to affect the class design in Mesa due to some needs/requirements from Mesa-Geo.

return x < 0 or x >= self.width or y < 0 or y >= self.height


class Cell(Agent):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the definition of Cell?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cells are building blocks of RasterLayer, and can have multiple attributes, depending on specific models. Some examples I wrote from the separated branch:

In theory, they can act like agents if the step() method is implemented, or stay static. But one thing I'm not sure (that I forgot to mention in the PR description) is how to make them work with the schedulers, which needs a reference/pointer to the model within the agent/cell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it is a container for attributes. It seems that it is going to be slow for huge raster grid size. Makes me wonder that the raster layer can be reimplemented in Cython for faster computation. But this can be done in future iterations.

@wang-boyu
Copy link
Member Author

This might be an opportunity for designing a powerful API like you'd see in Pandas DF col/row aggregation. E.g. min(self.space.get_neighboring_cells(pos, ...).apply(lambda x: x.color ** 2))

This is a good point! But I think I'll leave it to the next PR - to start with something like a CellGroupBy class that is in analogy to Pandas' GroupBy, due to the amount of work involved.

For this PR I'll remove the get_min_cell and get_max_cell methods. We can do these operations with min(self.space.get_neighboring_cells(pos, ...), key=lambda cell: cell.attribute) instead.

The get_min_cell and get_max_cell methods are now removed.

@rht
Copy link
Contributor

rht commented Jul 6, 2022

My remaining concerns are just documentation issue: you need to put into RasterLayer's docstring what you have said so far in the comments (which methods are copied from mesa.space.Grid, the diferences, and so on). Also the docstring for the Cell class.

@wang-boyu
Copy link
Member Author

Updated the docstrings.

@rht
Copy link
Contributor

rht commented Jul 7, 2022

LGTM. If @Corvince doesn't have the time to review, I will merge in 2 days from now.

@rht rht marked this pull request as ready for review July 7, 2022 03:24
@Corvince
Copy link
Contributor

Corvince commented Jul 7, 2022

Deal. I might or might not have the time, so a 2 days window sounds good.

@rht
Copy link
Contributor

rht commented Jul 10, 2022

2 days have passed. I'm merging so that Mesa-Geo is progressing.

@rht rht merged commit 9376f21 into projectmesa:master Jul 10, 2022
@wang-boyu wang-boyu deleted the feat/raster-layer branch July 11, 2022 03:45
@wang-boyu wang-boyu mentioned this pull request Nov 6, 2022
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.

None yet

3 participants