Refactor participant score #4011
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the participant score management from an optional field on Solution to a mandatory part of the participant state, introducing a type-state pattern that tracks scoring status through the type system: Unscored → Scored → Ranked. This eliminates the need for runtime score validation (expects/panics) by making invalid states unrepresentable at compile time.
Key Changes:
- Introduced
Scoredstate type containing a non-optional score, replacing theUnrankedtype - Moved score from
SolutiontoParticipantstate, making it type-safe - Renamed
Rankedenum toRankTypeand created a newRankedstate struct containing both rank type and score
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/autopilot/src/domain/competition/participant.rs |
Core refactor: Added Scored state type, split Ranked into RankType enum and Ranked state, added score() methods on Scored and Ranked, replaced set_score() with type-safe with_score() transition |
crates/autopilot/src/domain/competition/mod.rs |
Removed optional score field from Solution and its associated score() method, updated public exports to include new types |
crates/autopilot/src/domain/competition/winner_selection.rs |
Updated arbitration logic to use type-state transitions, refactored compute_scores_by_solution to return scored participants, updated all score access to use participant.score() instead of solution().score() |
crates/autopilot/src/run_loop.rs |
Updated type signatures to use Unscored instead of Unranked, changed score access from solution().score() to score() |
crates/autopilot/src/shadow.rs |
Updated imports and type signatures, changed score access pattern |
crates/autopilot/src/infra/persistence/mod.rs |
Updated score access from solution().score() to score() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| pub fn score(&self) -> Score { | ||
| self.score.expect( | ||
| "this function only gets called after the winner selection populated this value", |
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
Approved since this is already an improvement over the status quo.
| std::cmp::Reverse(participant.is_winner()), | ||
| // high score before low score | ||
| std::cmp::Reverse(participant.solution().score()), | ||
| std::cmp::Reverse(participant.score()), |
There was a problem hiding this comment.
Although it gets the job done having the score on the participant is a bit awkward IMO. This implies to me that the same solution submitted by different participants could have a different score.
There was a problem hiding this comment.
This implies to me that the same solution submitted by different participants could have a different score.
But each participant has a unique solution. Even if internally it has the same numbers, we don't share it across different participants, and the score will be the same with the old approach as well. So this doesn't change the logic.
There was a problem hiding this comment.
Yeah, the logic - or rather the outcome - didn't change. But I'm wondering where the correct place for the score is in the first place. For example if you were to store the solution in the driver instance the outcome would also not change but it would be a weird place to put the data, no?
OTOH the rules for scoring could be changed so the score is not really an inherent property of the solution either. Maybe the actual resolution here would be to rename Participant to Bid or something like that. This avoids the currently awkward situation where the same actual participant (i.e. driver) can show up multiple times in the participants vector. Also filtering out bids makes more sense than participants since we invalidate individual solutions and not ALL solutions of a particular participant. And it somewhat conveys the idea that the score of a solution depends on the rules encoded in the winner selection logic.
Overall I'm just bike shedding over the naming but the core essence of the product we built (ranking and picking solutions) should probably have the most appropriate names to avoid confusion.
(I'm aware that your PR did not suddenly make the name worse but it somewhat signalled that Participant wasn't a great name to begin with)
There was a problem hiding this comment.
Totally makes sense, thanks! But probably as a separate PR.
Description
Refactors the driver Participant struct's score field, which makes it non-optional by introducing new ranking types: Unscored and Scored, that also replace the Unranked type. The Ranked type now only contains participants with scores. One of the prerequisites for #4010 before extracting this logic into a separate crate.
Changes
How to test
Existing tests