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

FSRS wrongly converts a card to interday learning card? #3712

Open
user1823 opened this issue Jan 10, 2025 · 24 comments
Open

FSRS wrongly converts a card to interday learning card? #3712

user1823 opened this issue Jan 10, 2025 · 24 comments

Comments

@user1823
Copy link
Contributor

@L-M-Sherlock

I am using fsrs_short_term_with_steps. Many times, I notice that after a review, FSRS converts a intraday learning card into a interday learning card.

But, if the interval is 1d or more, shouldn't it simply convert the card to a review card?

Card info of one such card:

This is currently a interday learning card with a 1d interval.

@L-M-Sherlock
Copy link
Contributor

L-M-Sherlock commented Jan 15, 2025

According to the code, if the interval is greater than 0.5 day, the short_term is false, so the next stats should be ReviewState.

let (interval, short_term) = if let Some(states) = &ctx.fsrs_next_states {
(
states.good.interval,
(ctx.fsrs_short_term_with_steps_enabled || ctx.steps.is_empty())
&& states.good.interval < 0.5,
)
} else {
(ctx.graduating_interval_good as f32, false)
};
if short_term {
LearnState {
scheduled_secs: (interval * 86_400.0) as u32,
elapsed_secs: 0,
memory_state,
..self
}
.into()
} else {
ReviewState {
scheduled_days: ctx.with_review_fuzz(
interval.round().max(1.0),
minimum,
maximum,
),
ease_factor: ctx.initial_ease_factor,
memory_state,
..Default::default()
}
.into()
}

How did you know that the card is a interday learning card?

@L-M-Sherlock
Copy link
Contributor

I cannot reproduce this issue:

Image Image

@user1823
Copy link
Contributor Author

user1823 commented Jan 15, 2025

How did you know that the card is a interday learning card?

Because it appears as red in the deck picker and has an interval of 1d. I also verified that it has type = 1 in the database.

I can share the card from the backup created on 10th Jan. I don't have a backup before the last rating in the above screenshot.

After.apkg.zip

Parameters:
2.4860, 8.1461, 48.3187, 73.8801, 7.1577, 0.9967, 3.9325, 0.0010, 1.9119, 0.3130, 1.4081, 2.0449, 0.0167, 0.3757, 2.3958, 0.0000, 6.0000, 0.7905, 0.6943

Desired retention: 0.94

Edit:
I tried to reconstruct the "before" card by deleting the last revlog entry using debug console and then recalculating the memory states.

Before.apkg.zip

@L-M-Sherlock
Copy link
Contributor

I'm sorry but I cannot reproduce the bug. I tried to increase the desired retention to 0.99 and press good in the card. After several pressing, the card is converted to a review card.

@user1823
Copy link
Contributor Author

Ok, then let me guess.

For a stability of 14h and DR = 0.94, FSRS would have calculated an interval of about 8 hours. Since this is less than 0.5d, short_term would be true. But, at 23:12, the time left for day turnover (04:00) was only 4h and 48 min. So, Anki converted the interval to 1d.

@user1823
Copy link
Contributor Author

user1823 commented Jan 15, 2025

Ok, I ran the following commands in the debug console to regenerate the "before" card after deleting the revlogs and updating the memory states.

mw.col.db.execute("update cards set due=1736444160 where id = 1736346558840")
mw.col.db.execute("update cards set ivl=0 where id = 1736346558840")
mw.col.db.execute("update cards set queue=1 where id = 1736346558840")

Here's the result: Before.apkg.zip

To reproduce the issue, set your device time to 01:40 (night) on 10th Jan (Shanghai time). Launch Anki and set "next day starts at" to 6:30 (Shanghai time) (not really required because the default value will also reproduce the issue) and then rate Good.

Upon rating the card with the above steps, I get the following:

due = 1525
ivl = 0
queue = 3
type = 1
left = 1

So, in this case, Anki is indeed converting the intraday learning card (queue = 1) to an interday learning card (queue = 3).

@L-M-Sherlock
Copy link
Contributor

So, what's the expected behavior here? Should Anki keep the card in learning or convert it to review?

@user1823
Copy link
Contributor Author

user1823 commented Jan 15, 2025

If it remains a intraday learning card (queue = 1), would FSRS be able to know that I rated it on the next day so that it can calculate the next interval accordingly?

If yes, keep it as an intraday learning card.

If not, convert to review card.

My main problem with the current behaviour is that on the next day, this card has a very low R but it doesn't show up in the early part of the reviews (even with ascending R sort order) because Anki is trying to shuffle interday learning and review cards evenly.

@user1823
Copy link
Contributor Author

Spitballing, instead of changing the behaviour of FSRS, we can also solve this issue (and possibly others) by treating the interday learning cards and review cards in the same way during review sorting if Interday learning/review order is set to Mix with reviews

@L-M-Sherlock
Copy link
Contributor

If it remains a intraday learning card (queue = 1), would FSRS be able to know that I rated it on the next day so that it can calculate the next interval accordingly?

Yes. FSRS treats it as a review card if you review it in the next day.

@dae
Copy link
Member

dae commented Jan 26, 2025

Can this be closed?

@user1823
Copy link
Contributor Author

Not yet. I proposed two approaches for solving the issue. Please decide which would be easier to implement.

  • when the next learning step of a card is something like 6h but the day boundary is before that, keep the card as an intraday learning card (queue = 1) rather than converting it into a interday learning card (queue = 3)
  • treating the interday learning cards and review cards in the same way during review sorting if Interday learning/review order is set to Mix with reviews. Currently, Anki creates separate queues for them and then shuffles cards from each queue using the intersperser.

@L-M-Sherlock
Copy link
Contributor

L-M-Sherlock commented Feb 6, 2025

OK, I locate the related function:

pub(super) fn apply_learning_state(
&mut self,
current: CardState,
next: LearnState,
) -> RevlogEntryPartial {
self.card.remaining_steps = next.remaining_steps;
self.card.ctype = CardType::Learn;
if let Some(position) = current.new_position() {
self.card.original_position = Some(position)
}
self.card.memory_state = next.memory_state;
let interval = next
.interval_kind()
.maybe_as_days(self.secs_until_rollover());
match interval {
IntervalKind::InSecs(secs) => {
self.card.queue = CardQueue::Learn;
self.card.due = self.fuzzed_next_learning_timestamp(secs);
}
IntervalKind::InDays(days) => {
self.card.queue = CardQueue::DayLearn;
self.card.due = (self.timing.days_elapsed + days) as i32;
}
}
RevlogEntryPartial::new(
current,
next.into(),
self.card
.memory_state
.map(|d| d.difficulty_shifted())
.unwrap_or_default(),
self.secs_until_rollover(),
)
}

Here is the explanation of Claude:

Let me explain how the card queue changes:

In the apply_learning_state function, the card queue changes primarily based on the type of learning interval:

  1. When the interval is in seconds:
IntervalKind::InSecs(secs) => {
    self.card.queue = CardQueue::Learn;  // Set to learning queue
    self.card.due = self.fuzzed_next_learning_timestamp(secs);
}
  • Card is placed in the Learn queue
  • Due time is set to: current time + learning interval (with random fuzz)
  1. When the interval is in days:
IntervalKind::InDays(days) => {
    self.card.queue = CardQueue::DayLearn;  // Set to day learning queue
    self.card.due = (self.timing.days_elapsed + days) as i32;
}
  • Card is placed in the DayLearn queue
  • Due time is set to: days elapsed + learning interval days

The key logic is in the maybe_as_days function:

  • If the interval in seconds exceeds the remaining seconds in the current day (secs_until_rollover), it gets converted to days
  • Conversion formula: ((secs - secs_until_rollover) / 86_400) + 1
    • 86,400 is seconds in a day (24 * 60 * 60)
    • Adding 1 because counting starts from tomorrow

So in summary:

  • Cards with short learning intervals go into the Learn queue, with due time calculated in seconds
  • Cards with longer learning intervals go into the DayLearn queue, with due time calculated in days

@L-M-Sherlock
Copy link
Contributor

If I understand your first approach correctly, we should remove .maybe_as_days(self.secs_until_rollover());? It means, if the interval is in seconds, keep it in learning queue, otherwise, move it to day learning queue. But it will forbid card whose interval is 86400 to be moved to day learning queue.

@dae
Copy link
Member

dae commented Feb 6, 2025

As you're aware, even in SM2, Anki will put the card in the interday queue if it crosses a boundary, because queue selection is based on when the card will be shown again, not the length of the user's chosen learn step. Are you objecting to this behaviour in general, or are you making a case for FSRS behaving differently? I'd argue that the current behaviour is more appropriate, or perhaps moving it to the review queue, not the intraday queue. But I think the former might be a better choice, as if it's switched to a review card and the user forgets it, it will increment their lapse count.

@user1823
Copy link
Contributor Author

user1823 commented Feb 6, 2025

Are you objecting to this behaviour in general, or are you making a case for FSRS behaving differently?

I think that this behavior is problematic in general.

My main problem is that on the next day, Anki tries to mix these "interday" learning cards evenly with the "review" cards using the intersperser. But, if I have a large number of review cards due and I am not studying all of them, I can end up not seeing this "interday" learning card on the next day despite the fact that this card requires more urgent review.

The user can potentially solve the issue by using Show before reviews as the interday learning/review order (which I did after creating this issue) but that is not a perfect solution for those having multi-day steps (many SM-2 users).


I have said previously that altering the behavior of the intersperser might be the perfect solution (it will also allow interday learning cards to respect the sort orders) but altering the queue selection might be easier.

@L-M-Sherlock
Copy link
Contributor

L-M-Sherlock commented Feb 10, 2025

It's impossible to change the behavior of the intersperser because the sorting is done before intersperser and the intersperser doesn't know the sorting method. So the intersperser cannot sort the interday learning cards and review cards.

The intraday learning cards and review cards are gathered and sorted separately:

pub(super) fn gather_cards(&mut self, col: &mut Collection) -> Result<()> {
self.gather_intraday_learning_cards(col)?;
self.gather_due_cards(col, DueCardKind::Learning)?;
self.gather_due_cards(col, DueCardKind::Review)?;
self.gather_new_cards(col)?;
Ok(())
}
fn gather_intraday_learning_cards(&mut self, col: &mut Collection) -> Result<()> {
col.storage.for_each_intraday_card_in_active_decks(
self.context.timing.next_day_at,
|card| {
self.get_and_update_bury_mode_for_note(card.into());
self.learning.push(card);
},
)?;
Ok(())
}
fn gather_due_cards(&mut self, col: &mut Collection, kind: DueCardKind) -> Result<()> {
if self.limits.root_limit_reached(LimitKind::Review) {
return Ok(());
}
col.storage.for_each_due_card_in_active_decks(
self.context.timing,
self.context.sort_options.review_order,
kind,
self.context.fsrs,
|card| {
if self.limits.root_limit_reached(LimitKind::Review) {
return Ok(false);
}
if !self
.limits
.limit_reached(card.current_deck_id, LimitKind::Review)?
&& self.add_due_card(card)
{
self.limits.decrement_deck_and_parent_limits(
card.current_deck_id,
LimitKind::Review,
)?;
}
Ok(true)
},
)
}

@user1823
Copy link
Contributor Author

Well, nothing is impossible but I agree that it may be difficult. So, putting the card in the intraday queue would be the easiest solution?

@L-M-Sherlock
Copy link
Contributor

L-M-Sherlock commented Feb 10, 2025

If we keep learning cards in the intraday queue, learning cards and review cards are still in two separate queues. Does it solve your issue?

@user1823
Copy link
Contributor Author

Yes, because the learning card will be shown before the reviews which is desirable because it is the most urgent.

@L-M-Sherlock
Copy link
Contributor

I get it. I have a better solution: if the interval is shorter than 86400s or 43200s (instead of the seconds until rollover), keep it in the intraday queue. @dae, what do you think of?

@L-M-Sherlock
Copy link
Contributor

L-M-Sherlock commented Feb 10, 2025

In current implementation, if the current datetime is 2024-02-10 20:00 and the rollover is 6, the number of seconds until rollover is (4+6) * 60 * 60 = 36000. If the interval is 43200s (if DR=0.9, the stability is 0.5 day), it will be converted to 1 day. The true retention will drop from 90% to ~80+%. If we put the card in the mid or the end of the queue, it is bad.

@dae
Copy link
Member

dae commented Feb 10, 2025

I'm not very fond of the idea of putting cards due tomorrow in the intraday queue - it's what very old Anki did, and I don't think it's useful to keep the user waiting a few extra hours if they've already waited overnight.

As this issue is not FSRS-specific, and Anki already gives you the ability to put interday learning cards in front of reviews in the case of a backlog, I'm not sure this needs to be a priority right now? With the default settings, Anki would not even be introducing new cards while you have a backlog active.

@user1823
Copy link
Contributor Author

user1823 commented Feb 10, 2025

Maybe you are right. This is not a priority because this won't be a (major) problem with the default settings and users can avoid the problem by putting interday learning cards ahead of the reviews.

But, I think we should still keep this as a "low priority" issue for altering the behaviour of the intersperser when the interday learning and review cards are mixed. The final decision would be yours, though.

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

No branches or pull requests

3 participants