Migrate RasterLayer to mesa.discrete_space backend#307
Migrate RasterLayer to mesa.discrete_space backend#307falloficarus22 wants to merge 9 commits intomesa:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCell class now inherits from MesaDiscreteCell instead of Agent. RasterLayer refactored to use OrthogonalMooreGrid for cell storage and management. Constructor signature changed, type aliases added, and tests updated for dynamic attribute naming. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
==========================================
+ Coverage 77.67% 78.26% +0.59%
==========================================
Files 10 10
Lines 954 1003 +49
Branches 146 159 +13
==========================================
+ Hits 741 785 +44
- Misses 174 177 +3
- Partials 39 41 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
mesa_geo/raster_layers.py (3)
186-217:Cellnow carries two position attributes:coordinate(inherited fromMesaDiscreteCell) and_pos(legacy).Both are initialized to the same
(x, y)value, but nothing prevents them from diverging if someone modifies one without the other. Thepossetter (line 224) only updates_pos, leaving the inheritedcoordinatestale. Consider either:
- Making
posa read-only alias forcoordinate, or- Documenting that
coordinateis the canonical source andposis deprecated.This avoids a class of subtle bugs where code reads one but the other was modified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_geo/raster_layers.py` around lines 186 - 217, Cell currently maintains both inherited attribute coordinate and legacy _pos and the pos setter only updates _pos, allowing divergence; change pos to be a read-only alias of the canonical coordinate (return self.coordinate) and remove the pos setter, or if you must keep a setter, make it update both MesaDiscreteCell.coordinate and self._pos in sync (and mark pos as deprecated in the docstring), updating any use sites accordingly; modify the Cell class (pos property/getter/setter) to implement one of these two approaches and add a short deprecation note referencing Cell.pos, Cell._pos, and the inherited coordinate.
35-47: Missingfunctools.wrapson the wrapper function.Without
@functools.wraps(wrapped_function), the decorated functions (iter_cell_list_contents,get_cell_list_contents) lose their__name__,__doc__, and__module__attributes, which hurts debuggability and introspection.Proposed fix
+import functools + def accept_tuple_argument(wrapped_function: F) -> F: """Allow passing a single coordinate tuple to list-based cell APIs.""" + `@functools.wraps`(wrapped_function) def wrapper(grid_instance, positions, *args, **kwargs) -> Any:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_geo/raster_layers.py` around lines 35 - 47, The decorator accept_tuple_argument currently defines inner wrapper without preserving metadata; update it to apply functools.wraps(wrapped_function) to wrapper so decorated functions like iter_cell_list_contents and get_cell_list_contents retain their __name__, __doc__, and __module__; import functools if missing and place `@functools.wraps`(wrapped_function) directly above the wrapper definition (keeping the existing logic that converts a single (x,y) tuple into a list).
471-480: Auto-generated attribute naming scheme is unconventional.The progression
"attribute_0"→"attribute_0_1"→"attribute_0_2"is a bit confusing because"attribute_0_1"looks like a hierarchical sub-attribute. A simpler monotonic scheme like"attribute_0","attribute_1","attribute_2"would be more intuitive and align with common conventions.Suggested naming approach
- _default_attribute_name: str + _default_attribute_counter: int ... - self._default_attribute_name = "attribute_0" + self._default_attribute_counter = 0 ... def _default_attr_name() -> str: - base = self._default_attribute_name - if base not in self._attributes: - return base - suffix = 1 - candidate = f"{base}_{suffix}" - while candidate in self._attributes: - suffix += 1 - candidate = f"{base}_{suffix}" - return candidate + while True: + candidate = f"attribute_{self._default_attribute_counter}" + self._default_attribute_counter += 1 + if candidate not in self._attributes: + return candidate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mesa_geo/raster_layers.py` around lines 471 - 480, The current _default_attr_name generates names like "attribute_0_1" because it appends suffixes to the whole self._default_attribute_name; change it to derive a root by stripping a trailing numeric suffix from self._default_attribute_name (e.g. using a regex to get root and optional number) and then generate monotonic names root_0, root_1, root_2… by starting suffix at 0 (or 1 if you prefer) and incrementing until a candidate not in self._attributes is found; update the _default_attr_name function to use self._default_attribute_name (root) and self._attributes when building candidate names.tests/test_RasterLayer.py (1)
87-93:generated_attris misleading — the attribute name"val"is explicitly provided, not auto-generated.Since
attr_name="val"is passed on line 75, the attribute is always"val". Usingnext(iter(...))to retrieve it obscures this and makes the test harder to read. The previous direct-access pattern (e.g.,self.raster_layer.cells[0][1].val) was clearer here.If the intent is to test auto-generated names, a separate test case with
attr_name=Nonewould be more appropriate (and you already havetest_apply_raster_single_band_attr_name_nonefor that).Simplification
def test_apply_raster(self): raster_data = np.array([[[1, 2], [3, 4], [5, 6]]]) self.raster_layer.apply_raster(raster_data, attr_name="val") - generated_attr = next(iter(self.raster_layer.attributes)) - self.assertEqual(getattr(self.raster_layer.cells[0][1], generated_attr), 3) - self.assertEqual(self.raster_layer.attributes, {generated_attr}) + self.assertEqual(self.raster_layer.cells[0][1].val, 3) + self.assertEqual(self.raster_layer.attributes, {"val"}) self.raster_layer.apply_raster(raster_data, attr_name="elevation") self.assertEqual(self.raster_layer.cells[0][1].elevation, 3) - self.assertEqual(self.raster_layer.attributes, {generated_attr, "elevation"}) + self.assertEqual(self.raster_layer.attributes, {"val", "elevation"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_RasterLayer.py` around lines 87 - 93, The test uses a misleading variable generated_attr to access an attribute that is explicitly created with attr_name="val"; update the assertions in the block that calls self.raster_layer.apply_raster(..., attr_name="val") to directly reference the attribute name "val" (e.g., access self.raster_layer.cells[0][1].val and assert self.raster_layer.attributes includes "val") instead of using next(iter(...)) and generated_attr; if you need to test auto-generated names keep or create a separate test that calls RasterLayer.apply_raster with attr_name=None (you already have test_apply_raster_single_band_attr_name_none) so this case remains explicit and readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mesa_geo/raster_layers.py`:
- Around line 186-217: Cell currently maintains both inherited attribute
coordinate and legacy _pos and the pos setter only updates _pos, allowing
divergence; change pos to be a read-only alias of the canonical coordinate
(return self.coordinate) and remove the pos setter, or if you must keep a
setter, make it update both MesaDiscreteCell.coordinate and self._pos in sync
(and mark pos as deprecated in the docstring), updating any use sites
accordingly; modify the Cell class (pos property/getter/setter) to implement one
of these two approaches and add a short deprecation note referencing Cell.pos,
Cell._pos, and the inherited coordinate.
- Around line 35-47: The decorator accept_tuple_argument currently defines inner
wrapper without preserving metadata; update it to apply
functools.wraps(wrapped_function) to wrapper so decorated functions like
iter_cell_list_contents and get_cell_list_contents retain their __name__,
__doc__, and __module__; import functools if missing and place
`@functools.wraps`(wrapped_function) directly above the wrapper definition
(keeping the existing logic that converts a single (x,y) tuple into a list).
- Around line 471-480: The current _default_attr_name generates names like
"attribute_0_1" because it appends suffixes to the whole
self._default_attribute_name; change it to derive a root by stripping a trailing
numeric suffix from self._default_attribute_name (e.g. using a regex to get root
and optional number) and then generate monotonic names root_0, root_1, root_2…
by starting suffix at 0 (or 1 if you prefer) and incrementing until a candidate
not in self._attributes is found; update the _default_attr_name function to use
self._default_attribute_name (root) and self._attributes when building candidate
names.
In `@tests/test_RasterLayer.py`:
- Around line 87-93: The test uses a misleading variable generated_attr to
access an attribute that is explicitly created with attr_name="val"; update the
assertions in the block that calls self.raster_layer.apply_raster(...,
attr_name="val") to directly reference the attribute name "val" (e.g., access
self.raster_layer.cells[0][1].val and assert self.raster_layer.attributes
includes "val") instead of using next(iter(...)) and generated_attr; if you need
to test auto-generated names keep or create a separate test that calls
RasterLayer.apply_raster with attr_name=None (you already have
test_apply_raster_single_band_attr_name_none) so this case remains explicit and
readable.
|
Thanks for the fix. Removing Regarding this PR, do you mind narrowing it down to just fixing the |
2 participants
Summary
mesa-geofailed to import with current Mesa becausemesa.spaceis no longer available in Mesa 4 development versions. This blocked runtime usage immediately (import-time failure).This PR removes the legacy dependency and migrates
RasterLayerinternals to Mesa’s current cell-space architecture.Bug / Issue
Closes #306
import mesa_geoworks with current Mesa.RasterLayer, neighborhood methods, visualization paths) continue to behave as before.raster_layers.pyimported Coordinate andaccept_tuple_argumentfrommesa.space.mesa_georaisedModuleNotFoundError.Implementation
mesa.spacedependency fromraster_layers.py.mesa.discrete_space:Cellnow subclassesmesa.discrete_space.cell.Cell.RasterLayernow initializes an internalOrthogonalMooreGridand maps grid cells to existingself.cells[x][y]structure.accept_tuple_argumenthelper.test_RasterLayer.pyto avoid hardcoded runtime-dependent attribute names.Testing
mesa_geois now successful.Additional Notes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor