Skip to content
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.

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

Feat/support recency weighting #260

Merged
merged 10 commits into from
Dec 30, 2024
Merged

Feat/support recency weighting #260

merged 10 commits into from
Dec 30, 2024

Conversation

L-M-Sherlock
Copy link
Member

@L-M-Sherlock L-M-Sherlock commented Dec 19, 2024

@L-M-Sherlock L-M-Sherlock marked this pull request as ready for review December 24, 2024 03:50
@L-M-Sherlock L-M-Sherlock changed the title Feat/support recency weighting (WIP) Feat/support recency weighting Dec 24, 2024
@L-M-Sherlock L-M-Sherlock merged commit a7aaa40 into main Dec 30, 2024
3 checks passed
@L-M-Sherlock L-M-Sherlock deleted the Feat/recency-weighting branch December 30, 2024 12:08
@@ -210,29 +219,33 @@ impl<B: Backend> FSRS<B> {
if items.is_empty() {
return Err(FSRSError::NotEnoughData);
}
let weighted_items = recency_weighted_fsrs_items(items);
Copy link
Contributor

Choose a reason for hiding this comment

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

@L-M-Sherlock, shouldn't this be constant_weighted_fsrs_items?

In open-spaced-repetition/fsrs-optimizer#152 (comment), you said that evaluation doesn't use weights.

In addition to what @Expertium said, adding weights to the evaluation makes comparing RMSE in Anki 24.11 and 25.01 difficult.

I noticed this because Anki 24.11 shows Log loss: 0.2551, RMSE(bins): 2.91%. 64,368 reviews while Anki 25.01 shows Log loss: 0.2891, RMSE(bins): 3.18%. 64,368 reviews with the same parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be recency_weighted_fsrs_items otherwise Anki will prefer the old parameters.

Copy link
Contributor

@user1823 user1823 Jan 17, 2025

Choose a reason for hiding this comment

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

Well, I have changed my mind. Recency weighting makes training give more importance to recent reviews, which are those that the users care the most about.

If evaluation doesn't apply recency weighting, the more optimal parameters will produce worse metrics because the older reviews will have higher loss. So, Anki won't accept these new parameters.

But, we will need to inform users that the increased RMSE in 25.01 is due to a change in the evaluation method and this increase doesn't mean that FSRS has become worse for them.

Edit: You commented when I was typing this. 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt most users will check the RMSE after they update Anki. We just need to inform a small group of users if necessary.

Btw, RMSE(bins) cannot tell us much useful information. If we want to diagnose the algorithm, some calibration graphs in the section 4.2 is necessary: https://github.com/open-spaced-repetition/fsrs4anki/blob/main/fsrs4anki_optimizer.ipynb

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.

[TODO] support sample weights when training
3 participants