104 Add Grid model and segment_strokes utility function #123
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 98.16% 98.23% +0.07%
==========================================
Files 19 20 +1
Lines 1145 1191 +46
==========================================
+ Hits 1124 1170 +46
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Asanto32
left a comment
Asanto32
left a comment
There was a problem hiding this comment.
Some small comments, please look over the tests.
The major concern is about how padding is impacting col width/row height. I think you missed this because you only tested 2x2
| ) | ||
|
|
||
|
|
||
| @dataclasses.dataclass |
There was a problem hiding this comment.
I still feel like this is a potential future refactor point, too many of these dataclasses feel like regular classes (not just for data storage)
| y_max: float, | ||
| n_rows: int, | ||
| n_cols: int, | ||
| labels: Optional[List[str]] = None, |
There was a problem hiding this comment.
should labels be optional?
| padded_y_min = y_min - padding | ||
| padded_y_max = y_max + padding | ||
|
|
||
| col_width = (padded_x_max - padded_x_min) / n_cols |
There was a problem hiding this comment.
why do you take the padding into effect for the col/row size? it should only be for the start/ends correct? this padding was just for when a centroid lands on the outer boundary?
There was a problem hiding this comment.
Run this example and check: 3x3 grid, from [0,30] for x,y. now check you cell_x_min and cell_x_max for row,cols (1,2)
| y_max: Top boundary of the grid bounding box. | ||
| n_rows: Number of rows in the grid. | ||
| n_cols: Number of columns in the grid. | ||
| labels: Optional list of labels for each cell in row-major order. |
There was a problem hiding this comment.
As before should this be optional?
|
|
||
| for line_number, group in data.groupby("line_number"): | ||
| stroke = models.Stroke( | ||
| points=group.reset_index(drop=True), |
There was a problem hiding this comment.
what does reset_index do here?
| assert total_strokes == 3 | ||
|
|
||
| def test_stroke_points_are_correct(self) -> None: | ||
| """Each Stroke should contain the correct subset of points.""" |
There was a problem hiding this comment.
this only tests one stroke
| from graphomotor.core import models | ||
|
|
||
|
|
||
| class TestGridFromBbox: |
There was a problem hiding this comment.
once again why the class here?
| ) | ||
|
|
||
| assert len(grid.cells) == 1 | ||
| cell = grid.cells[0] |
There was a problem hiding this comment.
why is this after an assert?
| assert grid.cells[0].y_min > grid.cells[2].y_min | ||
| assert grid.cells[0].x_min < grid.cells[1].x_min | ||
|
|
||
| def test_labels_assigned(self) -> None: |
There was a problem hiding this comment.
these first three tests should all be combined probably
| """Tests for Grid.get_cell_for_point.""" | ||
|
|
||
| @pytest.fixture | ||
| def grid_2x2(self) -> models.Grid: |
There was a problem hiding this comment.
might be nice to make this extensible to NxN...
2 participants
Summary
Adds a
Griddataclass tomodels.pyand asegment_strokes()inalphabet_utils.py.Gridowns a list ofGridCellobjects and provides afrom_bboxfactory for subdivision andget_cell_for_pointfor lookup.segment_strokes()groups drawing data byline_number, createsStrokeobjects, and assigns each to a cell by centroid.Notes
from_bboxpads the outer grid edges by a small amount (default 0.1) so that centroids landing exactly on the boundaries are not excluded