search

open-spaced-repetition / fsrs-rs

FSRS for Rust, including Optimizer and Scheduler
https://crates.io/crates/fsrs
BSD 3-Clause "New" or "Revised" License
185 stars 19 forks source link

Request: Ignore reviews before "Forget" #113

Closed user1823 closed 1 year ago

user1823 commented 1 year ago

The following code removes revlogs before the last continuous group of LEARN type ratings. So, it (partially) deals with cards that have been reset ("Forget"). https://github.com/ankitects/anki/blob/afa84b5f5299eda1fda09c4f9571993aaebbc75b/rslib/src/scheduler/fsrs/weights.rs#L116

However, what if a card is reset while it is still in its learning stage? In this case, we will need to find the "Forget" ratings and remove the reviews before such ratings.

Secondly, the following code filters out reviews before the instances where the card goes from Review to Learning. But, isn't this case already dealt with when we are looking for the last continuous group of LEARN type ratings? So, I can't think of any reason to include the following code:

https://github.com/ankitects/anki/blob/afa84b5f5299eda1fda09c4f9571993aaebbc75b/rslib/src/scheduler/fsrs/weights.rs#L166-L176

Thirdly, does "last continuous group of LEARN type ratings" actually need to be a "group" (> 1 LEARN type ratings)? In that case, we will miss many cases where there was only a single LEARN rating.

By the way, I also put forward the first two issues in https://github.com/open-spaced-repetition/fsrs-rs/issues/63#issuecomment-1713839621. But, they were not addressed and I also forgot about them. I was reminded of the issues by https://forums.ankiweb.net/t/anki-23-10-release-candidate/35967/108.

L-M-Sherlock commented 1 year ago

Could you provide samples for that case?

user1823 commented 1 year ago

Could you provide samples for that case?

For the following one?

what if a card is reset while it is still in its learning stage?

If so, consider this review history:

L-M-Sherlock commented 1 year ago

In your case, the current implementation ignores the first and the second review.

user1823 commented 1 year ago

In your case, the current implementation ignores the first and the second review.

If you are sure about it, the first point is done. We can then focus on the 2nd and the 3rd points.

L-M-Sherlock commented 1 year ago

Secondly, the following code filters out reviews before the instances where the card goes from Review to Learning. But, isn't this case already dealt with when we are looking for the last continuous group of LEARN type ratings? So, I can't think of any reason to include the following code:

I have forgotten about that. You can ask @dae.

Thirdly, does "last continuous group of LEARN type ratings" actually need to be a "group" (> 1 LEARN type ratings)? In that case, we will miss many cases where there was only a single LEARN rating.

It doesn't. It allows the group size equal to one. You can read the code:

https://github.com/ankitects/anki/blob/afa84b5f5299eda1fda09c4f9571993aaebbc75b/rslib/src/scheduler/fsrs/weights.rs#L121C13-L144

dae commented 1 year ago

That code is based on your original code Jarrett :-) https://github.com/open-spaced-repetition/fsrs-rs/blob/c242a4929a73614c48bef8fdfa2ee1bf575cbb35/src/convertor.rs#L159. Happy to remove it if it's not necessary.

L-M-Sherlock commented 1 year ago

That code is based on your original code Jarrett :-)

My fault. Please feel free to remove it. There are some redundant code in my implementation.

user1823 commented 1 year ago

@dae, I am not very confident about it (because Rust code is difficult for me to decipher) but I think that the code dealing with the following review history is "fragile".

When a card is reset while it is still in its learning stage, for example, in the following review history, all the reviews up to (and including) the Manual rating should be filtered out.

  • Learn
  • Manual (Forget)
  • Learn
  • Learn
  • Review

In other words, I think that a bug can be introduced if https://github.com/ankitects/anki/blob/9268dce70790cce41c0f792e7305499a828ed3e9/rslib/src/scheduler/fsrs/weights.rs#L120-L147 is moved below https://github.com/ankitects/anki/blob/9268dce70790cce41c0f792e7305499a828ed3e9/rslib/src/scheduler/fsrs/weights.rs#L149-L157.

So, is it a good idea to add a test for this case?

dae commented 1 year ago

There are some tests at the bottom of the file - if you feel another is required, a PR would be welcome.

https://github.com/ankitects/anki/blob/9268dce70790cce41c0f792e7305499a828ed3e9/rslib/src/scheduler/fsrs/weights.rs#L317