Add Cell.xy for RasterLayer cell coordinates and clarify pos/indices semantics

[codebreaker32]

Summary

This PR introduces explicit coordinate attributes (cell.xy, cell.grid_pos, cell.rowcol) to Cell objects in RasterLayer. It disambiguates real-world geographic coordinates from internal grid indices and deprecates the ambiguous cell.pos and cell.indices properties.

Motive

Closes #298

Implementation

1. Updated mesa_geo/raster_layers.py:
2. Updated RasterLayer Initialization:
3. Test Suite Updates:

Usage Examples

Old Way (Ambiguous & Manual Calculation):

cell = raster_layer.cells[0][0]

print(cell.pos)  # Output: (0, 0)
transform = raster_layer.transform
real_x, real_y = transform * (cell.indices[1] + 0.5, cell.indices[0] + 0.5)

New Way:

cell = raster_layer.cells[0][0]

print(cell.xy)       # Output: (-122.45, 37.75)
print(cell.grid_pos) # Output: (0, 0)
print(cell.rowcol)   # Output: (10, 0)

Summary by CodeRabbit

• New Features

  • apply_raster / get_raster accept attr_name for named raster layers.
  • Added xy property exposing real-world cell center coordinates.

• Deprecations

  • Cell position accessors pos and indices deprecated in favor of grid_pos and rowcol; deprecated accessors emit warnings.

• Other

  • Per-cell attribute renamed to val; tests added/updated for deprecation warnings and coordinate transforms.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

Walkthrough

Refactors Cell and RasterLayer to add explicit xy, grid_pos, and rowcol attributes, deprecate legacy pos/indices with FutureWarnings, update cell construction and image indexing to use the new names, and expand RasterLayer tests and raster attribute handling (named attributes).

Changes

Cohort / File(s)	Summary
Core deprecation refactor mesa_geo/raster_layers.py	Cell.init extended to accept xy, grid_pos, rowcol with fallback from legacy pos/indices. Added deprecated pos and indices properties that emit FutureWarning and map to grid_pos/rowcol. RasterLayer now constructs cells with xy, grid_pos, rowcol. to_image reads cell.rowcol. Added warnings import.
Raster API & tests tests/test_RasterLayer.py	Tests updated for per-attribute raster support: apply_raster/get_raster support optional attr_name. Tests renamed cell data attribute_5→val, pos→grid_pos, and assert deprecation warnings for pos/indices. Added coordinate-transformation checks and multi-layer expectations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• add Cell.xy for RasterLayer cell coordinates and clarify pos/indices semantics #298: Implements adding xy and clarifying pos/indices semantics by introducing grid_pos and rowcol and deprecating the old accessors.

Poem

🐇 I hopped through grids both old and new,

xy shines where pos once grew,
grid_pos, rowcol learn the tune,
Warnings whisper, change comes soon,
A little rabbit cheers the view.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the main change: adding Cell.xy and clarifying pos/indices semantics through introducing grid_pos and rowcol properties.
Linked Issues check	✅ Passed	All coding requirements from issue #298 are implemented: cell.xy added for real-world coordinates, cell.grid_pos and cell.rowcol introduced, and pos/indices deprecated with warnings.
Out of Scope Changes check	✅ Passed	All changes align with issue #298 scope: coordinate attribute additions, deprecations, and test updates reflect the stated objectives without introducing unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codebreaker32]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/test_RasterLayer.py (1)

9-125: Consider adding tests for new Cell attributes and deprecation warnings.

The tests are correctly updated for the API changes, but coverage for the new functionality is incomplete:

1. No tests verify that cell.xy contains correct real-world coordinates
2. No tests verify that cell.rowcol contains correct row/column indices
3. No tests verify that accessing deprecated cell.pos and cell.indices emit FutureWarning

Example test for deprecation warning:

def test_deprecated_pos_warning(self):
    cell = self.raster_layer.cells[0][0]
    with self.assertWarns(FutureWarning):
        _ = cell.pos

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 199abcd and 46fdb04.

📒 Files selected for processing (2)

• mesa_geo/raster_layers.py
• tests/test_RasterLayer.py

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_RasterLayer.py (1)

mesa_geo/raster_layers.py (3)

• apply_raster (394-414)
• attributes (313-320)
• get_raster (416-442)

🔇 Additional comments (10)

mesa_geo/raster_layers.py (6)

11-11: LGTM!

The warnings import is correctly added to support the deprecation warnings implementation.

---

165-167: LGTM!

Clean class-level type annotations for the new coordinate attributes.

---

169-206: LGTM!

The __init__ method correctly implements backward compatibility by accepting both deprecated (pos, indices) and new (grid_pos, rowcol, xy) parameters. The fallback logic prioritizes new parameters while maintaining support for legacy code.

---

208-242: LGTM!

Well-implemented deprecation pattern:

• Uses FutureWarning (appropriate for user-facing deprecation)
• stacklevel=2 correctly points to the caller's code
• Both getters and setters emit warnings and delegate properly

---

300-309: LGTM!

Correct implementation of cell initialization:

• Real-world coordinates (xy) correctly computed using pixel center offsets (+0.5)
• Affine transform applied with proper (col, row) ordering
• grid_pos and rowcol are correctly derived from the loop variables and index calculations

---

604-604: LGTM!

Correctly updated to use cell.rowcol instead of the deprecated cell.indices, avoiding deprecation warnings in internal code paths.

tests/test_RasterLayer.py (4)

28-47: LGTM!

Test correctly updated to use named attributes (val, elevation) instead of auto-generated attribute names, and assertions properly reflect the expected attribute set.

---

52-77: LGTM!

Test properly exercises both named and auto-generated attributes, with clear comments explaining the expected 3 layers.

---

81-113: LGTM!

Test correctly updated to use grid_pos instead of the deprecated pos attribute for cell position assertions.

---

115-125: LGTM!

Test correctly updated to use grid_pos for the maximum cell position assertion.

[codebreaker32]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/test_RasterLayer.py (1)

28-28: Typo in test method name.

The method is named test_apple_raster but should likely be test_apply_raster based on what it tests.

✏️ Suggested fix

-    def test_apple_raster(self):
+    def test_apply_raster(self):

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46fdb04 and 85fa7dd.

📒 Files selected for processing (1)

• tests/test_RasterLayer.py

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_RasterLayer.py (1)

mesa_geo/raster_layers.py (8)

• apply_raster (394-414)
• attributes (313-320)
• get_raster (416-442)
• pos (209-215)
• pos (218-224)
• indices (227-233)
• indices (236-242)
• transform (117-125)

🔇 Additional comments (5)

tests/test_RasterLayer.py (5)

71-77: Potential order-sensitivity issue with set iteration.

The comment on line 72-73 states "order doesn't matter for equality check," but np.testing.assert_array_equal is order-sensitive. According to the relevant code snippet, get_raster() iterates over self.attributes which is a set[str]. Since all three raster layers have identical data in this test, the assertion passes regardless of order, but this could be fragile if the test data changes.

Consider whether this test should be more explicit about expected behavior, or if you intentionally rely on identical data to avoid order issues.

---

90-90: LGTM!

The updates from pos to grid_pos correctly align with the new API semantics. The test logic remains valid.

Also applies to: 99-99, 111-111

---

124-125: LGTM!

Property update to grid_pos is consistent with other test methods.

---

127-152: Good deprecation test coverage.

The test correctly validates that:

1. Deprecated properties emit FutureWarning
2. Getters delegate to the new property names
3. Setters also work with deprecation warnings

This aligns well with the implementation in raster_layers.py lines 208-241.

---

154-175: Well-structured coordinate transformation test.

The test effectively validates:

• grid_pos to rowcol mapping follows the expected formula (height - 1 - y, x)
• Cell center coordinates (xy) are correctly computed using the affine transform with (col + 0.5, row + 0.5)

The use of assertAlmostEqual is appropriate for floating-point coordinate comparisons.

[codebreaker32]

Please review @wang-boyu

[wang-boyu]

Thank you for your contribution. I put the issues as an idea and didn't expect it to be implemented anytime soon. But I'll try to find sometime and look into this PR.

[wang-boyu]

There's an ongoing discussion about multi-space suppport in Mesa and I'd like to see how they name similar concepts for a Cell in Mesa's future discrete space, to see whether we should follow Rasterio's name convention or not: mesa/mesa#2585 (reply in thread)

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[wang-boyu]

I spent a few days thinking about Cell's property names, mostly about generic cell.position and cell.index vs. raster specific cell.xy and cell.rowcol. Now I've updated this PR and here's a summary of what it does and why:

1. add cell.xy for location in CRS. This is to follow Rasterio's convention to use (x, y) for geographic/projected coordinates (see Transforms in Rasterio).

2. deprecate cell.indices to cell.rowcol. This is also to follow Rasterio's convention as in the above link.

3. keep cell.pos as is, which is cell's location in (grid_x, gird_y) format.

   I thought about deprecating cell.pos to cell.grid_pos as described in add Cell.xy for RasterLayer cell coordinates and clarify pos/indices semantics #298. I agree cell.grid_pos is clearer, and it could be done in similarly ways as deprecating cell.indices to cell.rowcol. However, there are also several neighborhod functions in raster layers that take a pos argument, such as

   get_neighborhood(self, pos: Coordinate, ...)

   For consistency, deprecating cell.pos would also mean to rename these parameters to grid_pos. While it is definitely possible, I find the name change from pos to grid_pos does not justify this much trouble. Hence, for now I choose to keep cell.pos and if still want to rename it to cell.grid_pos, we can do it later.

I'll keep this PR open for some more time in case there's anything else to fix before merging.

[EwoutH]

We might want to update our GIS examples based on this PR. I created a tracking issue:

• Update GIS example models for Cell API changes mesa-examples#326

@wang-boyu since this is (apparently) a breaking change:

• Can we do anything to make this change less breaking? (like first only warn)
• Have we documented migration properly?