Uh oh!
There was an error while loading. Please reload this page.
Add the terrain store: region grids and tile cache - #55
Conversation
Coverage Report for CI Build 33993292664Coverage increased (+0.7%) to 98.525%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
d974f1c to
1ea3053Compare
KurbyDoo
left a comment
There was a problem hiding this comment.
I changed some of the specifications of the original tickets since they don't really match what I'm thinking anymore, could you update this PR accordingly? Thanks
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
structure looks good overall!
please remove all comments that are not nessasary, it makes the code hard to read and confusing / misleading for future editors. ideally behaviour should be obvious from variable and function names and comments should only be used for when the reason for doing something is not clear. comments should also not reference issue numbers or talk about things that do not exist in the codebase / are not implemented yet
also add the coverage file to gitignore thanks
| """ | ||
| self._tiles[seed] = np.array(tile) | ||
| self._tiles.move_to_end(seed) | ||
| def _drop_least_recently_used(self) -> None: |
There was a problem hiding this comment.
rename this function to something more clear, right now its not obvious that this function does the capacity check in addition to dropping
| assert weights.sum() == 4 | ||
| def test_adding_the_same_window_twice_doubles_it() -> None: |
There was a problem hiding this comment.
this is incorrect behaviour we dont want to enforce in testing, in practice we never want to add the same window twice. remove this test, in a follow up, we will enforce not adding duplicate windows
| OTHER_SEED = 5678 | ||
| def a_store(tile_height: int = 4, tile_width: int = 4, capacity: int = 8) -> TerrainStore: |
There was a problem hiding this comment.
can you make this a pytest fixture instead? fixures are pytests way of naming objects that reset every test within a test suite
| "not built yet, see #56" | ||
| ) | ||
| sums, tile_weights = self.grids(seed, x, y) |
There was a problem hiding this comment.
what happens if we add a window that goes over the tile edge? shouldnt we be storing the weights and product in the adjacent tiles?
| def test_a_window_that_hangs_off_the_edge_is_refused(row: int, col: int) -> None: | ||
| """numpy slices clip rather than complain, so this has to be caught by hand.""" | ||
| grids = RegionGrids(4, 4) | ||
| def test_a_window_that_crosses_the_edge_of_a_tile_is_refused(row: int, col: int) -> None: |
There was a problem hiding this comment.
can you implement the behaviour for splitting windows across tiles, then change this test to validate that behaviour instead
| cache = TileCache(capacity=4) | ||
| tile = np.arange(9.0).reshape(3, 3) | ||
| def fill(store: TerrainStore, seed: int, x: int, y: int, value: float) -> None: | ||
| """Cover a whole tile with one value, so it can be read back.""" |
There was a problem hiding this comment.
I think there should be some way for this to be handled cleanly with pytest rather than making it a function, similar to fixtures, can you look in to this?
Summary
Implements the Terrain Store (#31) — all six sub-tickets in one PR, since
the two halves are small and share a module.
RegionGrids— scratch space for one region. Holds a sum grid and aweight grid, accumulates each window as
value × weightandweight,and divides one by the other in
heights(). Written to only by thesampler (Clean every window and add the results into the region grids #35).
TileCache— finished tiles by seed, LRU-evicted at a fixedcapacity. Touched only by generation orchestration (Generation Orchestration #36).
Adds numpy (2.5.2) as the project's first runtime dependency, per
CONTRIBUTING:
pyproject.tomlanduv.lockare both committed.Validation
scripts/quality-check.shpasses locally/testcommands were run and are passing (e.g./test gpu)27 tests in
tests/test_store.py, covering every assertion listed in#40–#45. The overlap test uses the worked 1×6 example from #31 directly.
Full suite is 68 passed.
Checklist
Additional Comments
Two decisions worth a second opinion:
putcopies the incoming array so the generator can reuse its buffer,but
gethands back the cache's own grid rather than a copy — copyingon every read would undo the point of caching. Callers should treat a
returned tile as read-only. Happy to change this if you would rather it
be defensive on both sides.
sub-ticket, so it is not implemented here.
Closes#40, #41, #42, #43, #44, #45