feat: Support all stroke versions #6
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for multiple stroke data formats by introducing an abstract Stroke base class with generator methods, allowing strokes to be represented either as separate x/y lists (StrokeDict) or as a list of coordinate tuples (StrokeTuple).
Key Changes:
- Introduced abstract
Strokebase class withx_generator()andy_generator()methods - Refactored existing
Strokeclass toStrokeDictas a concrete implementation - Added new
StrokeTupleclass for tuple-based coordinate representation - Updated path signature extraction code to use generator methods instead of direct attribute access
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| py_path_signature/data_models/stroke.py | Adds abstract Stroke base class and creates two concrete implementations: StrokeDict (refactored from original Stroke) and new StrokeTuple class |
| py_path_signature/path_signature_extractor.py | Updates bounding box calculation and coordinate-to-pixel conversion to use new generator methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class StrokeTuple(Stroke): | ||
|
|
||
| coordinates: Annotated[List[Tuple[float, float]], Field(min_length=2)] |
There was a problem hiding this comment.
The minimum length is set to 2, but StrokeDict requires min_items=1 for x and y lists. This inconsistency means StrokeTuple requires at least 2 coordinate pairs while StrokeDict can have just 1 point. Consider using min_length=1 for consistency across stroke formats.
| coordinates: Annotated[List[Tuple[float, float]], Field(min_length=2)] | |
| coordinates: Annotated[List[Tuple[float, float]], Field(min_length=1)] |
| for i in self.x: | ||
| yield i | ||
|
|
||
| def y_generator(self) -> Iterable[float]: | ||
| for i in self.y: | ||
| yield i | ||
|
|
There was a problem hiding this comment.
The generator simply iterates and yields items from self.x without transformation. This can be simplified to yield from self.x for better performance and cleaner code. The same applies to y_generator() on lines 54-56.
| for i in self.x: | |
| yield i | |
| def y_generator(self) -> Iterable[float]: | |
| for i in self.y: | |
| yield i | |
| yield from self.x | |
| def y_generator(self) -> Iterable[float]: | |
| yield from self.y |
| def x_generator(self) -> Iterable[float]: | ||
| for (x,y) in self.coordinates: | ||
| yield x | ||
|
|
||
| def y_generator(self) -> Iterable[float]: | ||
| for (x,y) in self.coordinates: | ||
| yield y |
There was a problem hiding this comment.
Both generators iterate over self.coordinates separately, which could be inefficient for large coordinate lists accessed sequentially. Consider if the calling code in path_signature_extractor.py could be refactored to iterate once and unpack coordinates, avoiding duplicate iteration.
| for (x,y) in self.coordinates: | ||
| yield x | ||
|
|
||
| def y_generator(self) -> Iterable[float]: | ||
| for (x,y) in self.coordinates: |
There was a problem hiding this comment.
Missing space after comma in tuple unpacking. Should be for (x, y) in self.coordinates: to follow PEP 8 style guidelines. Same issue on line 67.
| for (x,y) in self.coordinates: | |
| yield x | |
| def y_generator(self) -> Iterable[float]: | |
| for (x,y) in self.coordinates: | |
| for (x, y) in self.coordinates: | |
| yield x | |
| def y_generator(self) -> Iterable[float]: | |
| for (x, y) in self.coordinates: |
No description provided.