Fix/fallback to non-manual entry when first_of_last_learn_entries non found

[L-M-Sherlock]

fix #3634

[user1823]

Just to be sure, non_manual_entries.is_some() is true only if there are non-manual entries AFTER the Reset entry, right?

Did you try this on a card that has no entries after a reset entry?

[L-M-Sherlock]

Sure:

[L-M-Sherlock]

Wait a minute... The single_card_revlog_to_items is too complex than my thoughts... The current implementation uses the first non-manual entry instead of the first non-manual entry after the Reset entry. I will fix it.

[L-M-Sherlock]

@dae, sorry for bothering you, but I don't understand this snippet of code:

anki/rslib/src/scheduler/fsrs/params.rs

Lines 289 to 300 in d900506

	if !revlogs_complete {
	revlogs_complete = matches!(
	entries.first(),
	Some(RevlogEntry {
	review_kind: RevlogReviewKind::Manual,
	..
	}) | Some(RevlogEntry {
	review_kind: RevlogReviewKind::Rescheduled,
	..
	})
	);
	}

What's the aim of it?

[dae]

Introduced in db93939.

I believe the goal was to treat them as complete even if no learning entries have been found, if they started with a reschedule?

[user1823]

Dae added that code to implement my suggestion in open-spaced-repetition/fsrs4anki#540 (comment)

The issue: open-spaced-repetition/fsrs4anki#557

If the user uses Set Due Date to introduce a card rather than by normal way, the cards were getting very high difficulty.

Basically, it is the same issue that has been discussed many times in open-spaced-repetition/fsrs4anki#675.

[L-M-Sherlock]

My PR will infer the memory state from the first non-manual entry aftet Reset and set revlogs_complete to false. But this snippet of code will reverse my operation in some cases.

[user1823]

In open-spaced-repetition/fsrs4anki#675, I suggested removing this snippet. So, I think that you can remove it.

[L-M-Sherlock]

OK, here is the next problem:

anki/rslib/src/scheduler/fsrs/memory_state.rs

Lines 287 to 299 in d900506

	let first_review = entries
	.iter()
	// ignore manual and rescheduled revlogs and revlogs before the cutoff
	.find(|e| e.button_chosen > 0 && e.id.0 >= ignore_revlogs_before.0)
	.map(|e| FirstReview {
	interval: e.interval.max(1) as f32,
	ease_factor: if e.ease_factor == 0 {
	2500
	} else {
	e.ease_factor
	} as f32
	/ 1000.0,
	});

I think first_review should be extracted from the items returned by single_card_revlog_to_items instead of the original review entries.

anki/rslib/src/scheduler/fsrs/memory_state.rs

Lines 300 to 315 in d900506

	if let Some((mut items, revlogs_complete, _)) =
	single_card_revlog_to_items(entries, next_day_at, false, ignore_revlogs_before)
	{
	let mut item = items.pop().unwrap();
	if revlogs_complete {
	Ok(Some(FsrsItemWithStartingState {
	item,
	starting_state: None,
	}))
	} else if let Some(first_review) = first_review {
	// the revlog has been truncated, but not fully
	let mut starting_state = fsrs.memory_state_from_sm2(
	first_review.ease_factor,
	first_review.interval,
	historical_retention,
	)?;

The current code infers the memory state from the first_review which doesn't consider Reset entries.

[user1823]

I think first_review should be extracted from the items returned by single_card_revlog_to_items instead of the original review entries.

Your suggestion seems reasonable to me.

[L-M-Sherlock]

So this unit test should be removed.

anki/rslib/src/scheduler/fsrs/memory_state.rs

Lines 397 to 421 in d900506

	// but if there's only a single revlog entry, we'll fall back on current card
	// state
	let item = single_card_revlog_to_item(
	&fsrs,
	vec![RevlogEntry {
	ease_factor: 2500,
	interval: 100,
	..revlog(RevlogReviewKind::Review, 100)
	}],
	TimestampSecs::now(),
	0.9,
	0.into(),
	)?;
	assert!(item.is_none());
	card.interval = 123;
	card.ease_factor = 2000;
	card.ctype = CardType::Review;
	card.set_memory_state(&fsrs, item, 0.9)?;
	assert_int_eq(
	card.memory_state,
	Some(FsrsMemoryState {
	stability: 122.99994,
	difficulty: 7.334526,
	}),
	);

[L-M-Sherlock]

OK, the next problem:

https://github.com/L-M-Sherlock/anki/blob/35fa731b75b476ea730e7c2f38725d353143faef/rslib/src/scheduler/fsrs/params.rs#L328-L332

https://github.com/L-M-Sherlock/anki/blob/35fa731b75b476ea730e7c2f38725d353143faef/rslib/src/scheduler/fsrs/params.rs#L266-L272

If we find a relearning entry, should we set the revlogs_complete to true?

[user1823]

If we find a relearning entry, should we set the revlogs_complete to true?

What did the previous code do? I will have to search through previous discussions for reasoning for/against this. Unfortunately, I won't be able to devote much time today and tomorrow.

[user1823]

On a gross overview, the changes made till now look good to me.

[L-M-Sherlock]

Here is the previous code:

anki/rslib/src/scheduler/fsrs/params.rs

Lines 327 to 338 in d900506

	let first_relearn = entries
	.iter()
	.enumerate()
	.find(|(_idx, e)| {
	e.id.0 > ignore_revlogs_before.0 && e.review_kind == RevlogReviewKind::Relearning
	})
	.map(|(idx, _)| idx);
	if let Some(idx) = first_of_last_learn_entries.or(first_relearn) {
	// start from the (re)learning step
	if idx > 0 {
	entries.drain(..idx);
	}

It was introduced in 90b0c6d

[user1823]

If the previous code set it to true, then let's keep it that way. There must be some reason why it was done like that.

[L-M-Sherlock]

If the previous code set it to true, then let's keep it that way. There must be some reason why it was done like that.

In previous code, revlogs_complete is true only when Anki finds the first_of_last_learn_entries or the first entry is Manual or Rescheduled.

[L-M-Sherlock]

If revlogs_complete is false, Anki will infer the memory state from the relearn entry. If it's true, the rating of the relearn entry will be treated as the first rating.

[user1823]

If we find a relearning entry, should we set the revlogs_complete to true?

My initial answer would be NO.

But, what does 90b0c6d do?

[user1823]

Does this PR also fix #2921?

[L-M-Sherlock]

Does this PR also fix #2921?

Yes. The starting_state is converted from the first review of that affected card.

[L-M-Sherlock]

But, what does 90b0c6d do?

If the first_of_last_learn_entries is non-found, Anki will try to find the first_relearn_entry. If it's found, Anki will remove all entries before this entry. In the commit version, the first_relearn_entry was treated as the first learning entry. Its rating was used to generate the initial stability and difficulty.

[user1823]

Its rating was used to generate the initial stability and difficulty.

Using normal formula or using memory_state_from_sm2? Let's keep doing whatever Anki was doing before this PR. It would have better if we had a link to discussion in the commit message of 90b0c6d.

[L-M-Sherlock]

Using normal formula or using memory_state_from_sm2?

Using normal formula. But it was changed in db93939. This commit introduced revlogs_complete. And revlogs_complete is true only when last_learn_entry is found. If revlogs_complete is false, Anki will use memory_state_from_sm2.

[user1823]

But it was changed in db93939.

Well, if this is what has happened, using memory_state_from_sm2 was a bug. But, this change was made 1 year ago and no one has complained. So, let's keep revlogs_complete as False in that case for now.

[user1823]

Next issue/suggestion:

We should also exclude cram revlogs (entry.review_kind == RevlogReviewKind::Filtered && entry.ease_factor == 0) here.

        if matches!(entry.button_chosen, 1..=4) {
            non_manual_entries = Some(index);
        }

This is the last suggestion from me for today. I will take another look tomorrow to ensure that no issue is remaining and then we can request dae for a final review.

By the way, thanks for this PR. You have squashed tons of bugs with this.

[L-M-Sherlock]

I refactored a lot of codes.

Cram revlogs are skipped

    for (index, entry) in entries.iter().enumerate().rev() {
        if entry.review_kind == RevlogReviewKind::Filtered && entry.ease_factor == 0 {
            continue;
        }

Only search the first non-manual revlog entry after ignore date

        if matches!(entry.button_chosen, 1..=4) && entry.id.0 > ignore_revlogs_before.0 {
            non_manual_entries = Some(index);
        }

Fallback to non-manual revlog entry if the first of last learn entires is before ignore date

I referred to your idea: #2922 (comment)

        // While reviewing if the first learning step is before the ignore date,
        // fallback to non_manual_entries
        if let Some(idx) = first_of_last_learn_entries {
            if entries[idx].id.0 < ignore_revlogs_before.0 && idx < entries.len() - 1 {
                revlogs_complete = false;
                first_of_last_learn_entries = None;
            }
        }

[dae]

Sorry, I forgot we iterate on the entries in reverse, which I think has made my renaming incorrect. Reviewing...

[dae]

Ok, sorry about the delay. Tests past after the changes in the last commit, but I'm not 100% confident the behaviour is identical - can you spot any problems?

[L-M-Sherlock]

For this case, the revlog_complete should be false.

L | L R

[user1823]

The last commit (7fd9a77) has introduced a functional change (I think) and it isn't desirable.

After the above commit, if a card has revlog entries like L L | L R R, this card

• is used for training
• gets it memory states using normal FSRS formulas (instead of memory_states_from_sm2) at the L entry just after the cutoff.

where | = cutoff date

Maybe, this type of revlog entry requires a unit test because it is too easy to break.

[user1823]

Apart from the above mentioned functional issue, the code looks good to me.

However, I have some suggestions regarding wording:

memory_state.rs

• rename first_non_manual_entry to first_user_graded_entry (or something like that)
• change this comment

        // but if there's only a single revlog entry, we'll fall back on the first
        // non-manual entry

to

        // cards with a single review-type entry also get memory states from revlog rather than card states

params.rs

• change this comment

        // While reviewing, if the first learning step is before the ignore date,
        // we won't start from the learning step.

to

        // While reviewing, if the first learning step is before the ignore date,
        // infer memory states from sm2 at the revlog entry just after.

• change this comment

        // if there are no learning entries, but the user has reviewed the card,
        // we ignore all entries before the first grade

to

        // ignore all entries before the first user-graded rating matched by the loop

[dae]

Thank you both, I've reverted my changes and applied some of the other suggestions.