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

Fix/Relearning cards' Intervals don't update after changing Desired Retention #3236

Conversation

L-M-Sherlock
Copy link
Contributor

fix #3234

@user1823
Copy link
Contributor

Though I am not 100% sure, I think that your fix forces the recalculation of the interval every time a relearning card leaves the RELEARN state. If it is so, I think that the following fix is more efficient.

My idea is that Anki should clear the stored value of the interval of relearning cards (like d9f5487) when certain operations (listed below) are performed and then recalculate the interval during review if it is missing (because it was cleared).

The stored value of interval of relearning cards should be cleared when:

  • the desired retention is changed
  • the card is moved into different deck

I am not sure if we need to treat LEARN cards in the same way.

@L-M-Sherlock
Copy link
Contributor Author

My idea is that Anki should clear the stored value of the interval of relearning cards

This implementation would be complex.

Though I am not 100% sure, I think that your fix forces the recalculation of the interval every time a relearning card leaves the RELEARN state.

It's necessary if we update to FSRS-5 because it considers the short-term schedule.

@@ -52,6 +52,14 @@ impl RelearnState {
},
}
.into()
} else if let Some(states) = &ctx.fsrs_next_states {
Copy link
Contributor

@user1823 user1823 Jun 22, 2024

Choose a reason for hiding this comment

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

Does this work if there are no memory states at that point of time (e.g. the deck was changed, which clears the memory states)?

Copy link
Member

Choose a reason for hiding this comment

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

The state is recalculated prior to that point.

@dae dae merged commit 79917bb into ankitects:main Jun 22, 2024
1 check passed
@L-M-Sherlock L-M-Sherlock deleted the Fix/Relearning-cards'-Intervals-don't-update-after-changing-Desired-Retention branch June 22, 2024 11:12
@user1823
Copy link
Contributor

user1823 commented Jun 24, 2024

@L-M-Sherlock

Not 100% sure if this is a bug, but still worth looking into:

A relearning card on AnkiDroid (24.06.2 backend) shows the intervals 20m 30m 6d 7d

The same card on Anki 24.06.3 rc1 shows the intervals 20m 30m 7d 7d

The change of Good interval from 6d to 7d may be caused by something like open-spaced-repetition/fsrs4anki-helper#343 (different fuzz is applied because the reps count has increased by 1 due to the last Again rating)

But, should the Easy button have the same interval as the Good button?

Card Info:

Card: A.apkg.zip (rename to remove .zip)

@L-M-Sherlock
Copy link
Contributor Author

L-M-Sherlock commented Jun 25, 2024

Seems I should set the interval as states.easy.interval plus one?

fn answer_easy(self, ctx: &StateContext) -> ReviewState {
let scheduled_days = if let Some(states) = &ctx.fsrs_next_states {
let (minimum, maximum) = ctx.min_and_max_review_intervals(1);
let interval = states.easy.interval;
ctx.with_review_fuzz(interval as f32, minimum, maximum)
} else {
self.review.scheduled_days + 1
};
ReviewState {
scheduled_days,
elapsed_days: 0,
memory_state: ctx.fsrs_next_states.as_ref().map(|s| s.easy.memory.into()),
..self.review
}
}

FSRS gives the same interval for good and easy in current case because FSRS doesn't consider the same-day reviews except the first one.

@user1823
Copy link
Contributor

Seems I should set the interval as states.easy.interval plus one?

Maybe, yes

dae pushed a commit that referenced this pull request Jun 28, 2024
#3256)

* Fix FSRS easy interval being same as good interval in relearning cards

#3236 (comment)

* Update relearning.rs

* Update relearning.rs

* Set min interval of easy to Good + 1

* Ensure minimum doesn't exceed maximum (dae)

With a maximum interval set, it would be possible to confuse with_review_fuzz()
by passing min > max.
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.

Intervals don't update after changing Desired Retention
3 participants