Calculate_think_time#122
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 98.16% 98.21% +0.04%
==========================================
Files 19 19
Lines 1145 1177 +32
==========================================
+ Hits 1124 1156 +32
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ], | ||
| {"B": ("B", 5.0, 5.0, 1.0)}, | ||
| [{"label": "A", "order": 1}, {"label": "B", "order": 2}], | ||
| [("B", "C", 3.0, "B")], |
There was a problem hiding this comment.
Could we make these expected values have some description to them by either adding a description field or like an inline comment? the id does explain the scenario, but it doesn't explain how the 3.0 was derived.
| ], | ||
| {"B": ("B", 5.0, 5.0, 1.0)}, | ||
| [{"label": "A", "order": 1}, {"label": "B", "order": 2}], | ||
| [], |
There was a problem hiding this comment.
pretty much the same comment here as line 153, could we include a description field in the test parameters that explains why no think time is expected?
| assert seg.think_time == think_time | ||
| assert seg.think_circle_label == circle_label | ||
| else: | ||
| assert seg.think_time == 0.0 |
There was a problem hiding this comment.
shouldn't we also check the default think_circle_label which I guess would be an ""
| assert [seg.start_label for seg in result] == ["A", "B"] | ||
|
|
||
|
|
||
| def test_empty_segments_returns_empty() -> None: |
There was a problem hiding this comment.
How about when segments[0].points is empty? we have a guard for it in line 129 on time.py, I feel like we would need a test for that too.
| @@ -1,5 +1,9 @@ | |||
| """Feature extraction module for time-based metrics in trails drawing data.""" | |||
|
|
|||
| from typing import Any, Dict, List, Optional | |||
There was a problem hiding this comment.
Very nitty gritty but calculate_total_error_time uses dict[str, float] for type hints, so we could keep constistancy.
| def _find_circle_entry_time( | ||
| points: pd.DataFrame, circle: models.CircleTarget | ||
| ) -> Optional[float]: | ||
| """Find when a point last entered the circle by scanning backwards.""" |
There was a problem hiding this comment.
I think we could use a little more detail to this docstring. This returns the seconds value from the last sampled point that's inside the circle, which is kind of different from calculating or interpolating the boundary-crossing time, which I think this could be interpreted as. We could also just change the function name to something like find_last_time_inside_circle or something like that
| def _find_circle_exit_time( | ||
| points: pd.DataFrame, circle: models.CircleTarget | ||
| ) -> Optional[float]: | ||
| """Find when a point first left the circle by scanning forward.""" |
There was a problem hiding this comment.
Same comment as above (line 62), but I also think that we should explain the fallback behavior in the docstring as well.
| """ | ||
| trail_circles = circles[trail_id] | ||
| circle_order = {item["label"]: item["order"] for item in config[trail_id]["items"]} | ||
| segments = sorted(segments, key=lambda s: circle_order.get(s.start_label, 999)) |
There was a problem hiding this comment.
Could we use float("inf") instead of 999 as sort key fallback? or maybe keep it and put it in the docstring with a logger warning?
| next_seg.think_time = exit_time - entry_time | ||
| next_seg.think_circle_label = circle_label | ||
|
|
||
| if not segments or len(segments[0].points) == 0: |
There was a problem hiding this comment.
Could we turn these nested ifs into early return guards?
kimit0310
left a comment
kimit0310
left a comment
There was a problem hiding this comment.
Just a few small things + the nested ifs vs early return guards thing.
Need to verify this again later
3 participants
Resolves #73
Broke up longer function from https://github.com/theBasicScientist/trailsab-proc/blob/main/trails_analyzer.py#L86 into private functions for calculating circle entry time and exit time. Tests behavior for calculating intermediate think times and separate tests for edge cases.