Skip to content

fix: rating running average folded in the previous vote instead of the new one #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
grunch merged 1 commit into from
Aug 14, 2026
Merged

fix: rating running average folded in the previous vote instead of the new one #163

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 88 additions & 1 deletion src/user.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl User {
self.min_rating = rating.into();
} else {
self.total_rating =
old_rating + ((self.last_rating as f64) - old_rating) / (self.total_reviews as f64);
old_rating + ((rating as f64) - old_rating) / (self.total_reviews as f64);
Comment on lines 131 to +132

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Migrate persisted averages before applying the new recurrence

When an existing user has ratings written by an earlier release, total_rating is not yet an average containing all total_reviews: the old recurrence omitted the latest rating and folded in the first rating twice. Applying the incoming rating directly therefore permanently skips that pending historical vote. For example, a persisted user rated 5 then 1 has total_rating = 3.75; receiving 5 after this upgrade produces 4.1667 here, whereas the documented weighting gives 2.8333. Existing rows need to be migrated/rebuilt (or explicitly transitioned using historical rating data) before this recurrence is used.

Useful? React with 👍 / 👎.

@grunch grunch Aug 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good observation — the math checks out. The old recurrence telescopes to total_rating = (r1/2 + r1 + r2 + … + r(n-1))/n, so every persisted aggregate double-counts the first vote and omits the latest one, and your 3.75 → 4.1667 vs 2.8333 example is exactly right. A few clarifications on why this PR doesn't (and can't) address it here:

  1. An exact migration is not possible with the persisted data. The per-user error is exactly (r1 − last_rating)/n. last_rating is stored on the row, but r1 (the first vote) is not — the DB only keeps aggregates (total_rating, total_reviews, last_rating, min_rating, max_rating), not the vote history. There is nothing to rebuild from at this layer.

  2. The residual error is bounded and self-correcting. The absolute error in the implied sum is fixed (r1 − last_rating, at most ±4), so the average is off by at most 4/n at upgrade time and decays as 1/m as new votes are folded in correctly. The new recurrence is contractive over the historical error; it does not amplify it.

  3. Scope: this crate has no storage. mostro-core is a library; the SQLite rows live in mostrod. Any transition logic belongs in a daemon migration, not here — and holding this fix back would keep corrupting new data while the old data is discussed.

A practical follow-up for mostrod would be a one-time migration that folds in the pending stored vote once:

UPDATE users
SET total_rating = total_rating + (last_rating - total_rating) / total_reviews
WHERE total_reviews > 1;

That removes the omitted-latest-vote half of the error, leaving only the double-counted first vote, which is unrecoverable from the persisted aggregates. I'll open an issue on mostrod proposing it.

if self.max_rating < rating.into() {
self.max_rating = rating.into();
}
Expand All @@ -141,3 +141,90 @@ impl User {
self.last_rating = rating.into();
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn first_vote_is_weighted_by_half() {
let mut user = User::default();

user.update_rating(5);

assert_eq!(user.total_reviews, 1);
assert_eq!(user.total_rating, 2.5);
assert_eq!(user.last_rating, 5);
assert_eq!(user.max_rating, 5);
assert_eq!(user.min_rating, 5);
}

#[test]
fn second_vote_is_folded_into_the_average() {
let mut user = User::default();
user.update_rating(5);

user.update_rating(1);

// First vote weighted 1/2 -> 2.5, then incremental average with the
// new vote: 2.5 + (1 - 2.5) / 2 = 1.75
assert_eq!(user.total_reviews, 2);
assert!((user.total_rating - 1.75).abs() < 1e-9);
assert_eq!(user.last_rating, 1);
}

#[test]
fn low_vote_lowers_a_high_average() {
let mut user = User::default();
for _ in 0..10 {
user.update_rating(5);
}
let farmed_average = user.total_rating;
assert!((farmed_average - 4.75).abs() < 1e-9);

user.update_rating(1);

// Correct running average: (2.5 + 9 * 5 + 1) / 11 = 48.5 / 11
let expected = 48.5 / 11.0;
assert!(
user.total_rating < farmed_average,
"a 1-star review must lower the average, got {} (was {})",
user.total_rating,
farmed_average
);
assert!((user.total_rating - expected).abs() < 1e-9);
assert_eq!(user.last_rating, 1);
assert_eq!(user.min_rating, 1);
assert_eq!(user.max_rating, 5);
}

#[test]
fn high_vote_raises_a_low_average() {
let mut user = User::default();
user.update_rating(1);
user.update_rating(1);
let low_average = user.total_rating;

user.update_rating(5);

assert!(
user.total_rating > low_average,
"a 5-star review must raise the average, got {} (was {})",
user.total_rating,
low_average
);
// (0.5 + 1 + 5) / 3
assert!((user.total_rating - 6.5 / 3.0).abs() < 1e-9);
}

#[test]
fn min_and_max_track_extremes() {
let mut user = User::default();
user.update_rating(3);
user.update_rating(5);
user.update_rating(1);

assert_eq!(user.max_rating, 5);
assert_eq!(user.min_rating, 1);
}
}