Skip to content

60 task write trails drawing feature functions total distance updated#118

Merged
cgmaiorano merged 45 commits into
mainfrom
60-task-write-trails-drawing-feature-functions-total_distance_updated
Mar 10, 2026
Merged

60 task write trails drawing feature functions total distance updated#118
cgmaiorano merged 45 commits into
mainfrom
60-task-write-trails-drawing-feature-functions-total_distance_updated

Conversation

@cgmaiorano
Copy link
Copy Markdown
Collaborator

This PR closes issues #82 #60 #72 .

compute_segment_metrics belongs to LineSegment and calls all feature classmethods associated with LineSegment (valid_ink_trajectory, calculate_velocity_metrics, calculate_path_optimality, calculate_smoothness, detect_hesitations). It also calculates the ink_time metric.

@cgmaiorano cgmaiorano linked an issue Feb 11, 2026 that may be closed by this pull request
4 tasks
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.14%. Comparing base (f8a3e1d) to head (a4ebe58).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
- Coverage   98.61%   98.14%   -0.47%     
==========================================
  Files          19       19              
  Lines        1080     1131      +51     
==========================================
+ Hits         1065     1110      +45     
- Misses         15       21       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cgmaiorano cgmaiorano marked this pull request as ready for review February 11, 2026 17:39
Copy link
Copy Markdown
Collaborator

@Asanto32 Asanto32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions on this one with why we are using patching in the tests, and then some comments about re-arranging the if/elif chain to improve readability

Comment thread src/graphomotor/core/models.py
Comment thread tests/unit/test_models.py
}
}

with patch.object(segment, "valid_ink_trajectory", return_value=(0, 4)):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to patch for these next few tests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because valid_ink_trajectory determines if a trajectory exists from a start circle to another but the circles and point data I am using are arbitrary and it would fail if I let it run in these tests

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know valid_ink_trajectory works, I want to make sure the outer function works

Comment thread src/graphomotor/core/models.py Outdated
Comment thread src/graphomotor/core/models.py Outdated
Comment thread tests/unit/test_models.py Outdated
@cgmaiorano cgmaiorano requested a review from Asanto32 March 5, 2026 19:43
Comment thread src/graphomotor/core/models.py Outdated

import dataclasses
import datetime
from asyncio.log import logger
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a logger in config.py, if you look at some of the spiral modules, spiral_orchestrator for example you can see how we were using it

ink_start_idx, ink_end_idx = self.valid_ink_trajectory(start_circle, end_circle)

if ink_start_idx is None:
logger.warning(
Copy link
Copy Markdown
Collaborator

@Asanto32 Asanto32 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should exit and return right? I prefer that over the long if/elif chain.

if `ink_start_idx` is None:
  logger.warning()
  return
if `ink_end_idx` is None:
  self.ink_points = ...
    if len > 2
      do stuff;
    else:
      logger.warning()
      return
etc.

Let me know if this still isn't clear, we can chat about it 1-1

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was more compressed the other way, I still have to do these checks but since you wanted it to be more readable I broke it all up

@cgmaiorano cgmaiorano requested a review from Asanto32 March 6, 2026 15:03
Copy link
Copy Markdown
Collaborator

@Asanto32 Asanto32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cgmaiorano cgmaiorano merged commit fc90593 into main Mar 10, 2026
24 checks passed
@cgmaiorano cgmaiorano deleted the 60-task-write-trails-drawing-feature-functions-total_distance_updated branch March 10, 2026 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task: Write Trails Drawing Feature functions: total_distance

2 participants