Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two new sb convenience utilities to extract possession-based event sequences around a given trigger event, supporting common possession-context analysis workflows.
Changes:
- Added
get_possession_sequence(match_id, event_id, exclude_trigger=True)to return events with the samepossessionas the trigger event. - Added
get_prior_possession(match_id, event_id)to return events in the immediately preceding possession (possession - 1). - Added unit tests validating return types, possession filtering, trigger inclusion/exclusion, and sort ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
statsbombpy/sb.py |
Introduces two new public sequence-reconstruction helpers based on the events feed’s possession field. |
tests/statsbombpy_test/test_sb.py |
Adds test coverage for the new helpers (including unknown event_id behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| possession_id = trigger.iloc[0]["possession"] | ||
| sequence = all_events[all_events["possession"] == possession_id] | ||
| if exclude_trigger: | ||
| sequence = sequence[sequence["id"] != event_id] | ||
| return sequence.sort_values(["period", "timestamp"]).reset_index(drop=True) |
| possession_id = trigger.iloc[0]["possession"] | ||
| if possession_id <= 1: | ||
| return pd.DataFrame() | ||
| prior = all_events[all_events["possession"] == possession_id - 1] | ||
| return prior.sort_values(["period", "timestamp"]).reset_index(drop=True) |
| def _shot_id_and_possession(self): | ||
| evs = sb.events(match_id=self.MATCH_ID, flatten_attrs=True) | ||
| shot = evs[evs["type"] == "Shot"].iloc[0] | ||
| return shot["id"], int(shot["possession"]) |
There was a problem hiding this comment.
Hi, thanks for submitting this PR. The issue highlighted by the Copilot review when this PR was submitted will need to be fixed before approving this PR. As this is a helper function it should be included in helpers.py rather than in sb.py.
If you can make these changes and also address the conflicts I can review again. Thanks!
Both take the events DataFrame returned by sb.events() instead of fetching a match, and sort by (period, timestamp, index) so events sharing a timestamp keep their feed order. Tests use a synthetic DataFrame and make no network calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bf77dfb to
020107e
Compare
![orca-security-us[bot]](https://avatars.githubusercontent.com/in/276250?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
|
Thanks for the review. Both functions are now in helpers.py and take the events DataFrame from sb.events() instead of a match_id, so they match the other helpers and don't call the API. They sort by (period, timestamp, index), which addresses the Copilot comment, and the tests moved to test_helpers.py with an in-memory DataFrame, so they no longer fetch match data. I rebased onto master, which cleared the conflict. One note: 16 tests in the full suite fail on master as well, without this change (they need credentials, request open-data files that are no longer published, or patch statsbombpy.public.req, which doesn't exist). test_helpers.py passes in full. |
Adds two helpers for reconstructing event sequences around a match event, a common pattern in possession-based analysis.
Both take the events DataFrame from sb.events(), sort by (period, timestamp, index), and return an empty DataFrame if the event isn't found.
Tests are in test_helpers.py and use a small in-memory DataFrame, so they make no network calls. They include two events with the same timestamp to check the index tie-break.
Relates to #68.