Feat/FSRS Simulator

[L-M-Sherlock]

Preview:

TODO:

• add charts to visualize simulation results
• convert the memory state, due and last due of learning cards correctly
• move the simulator to a single page

[Expertium]

convert the memory state, due and last due of learning cards correctly

This would require using real deck sizes, right?

[L-M-Sherlock]

This would require using real deck sizes, right?

I plan to rename deck size to something like the number of new cards to add.

[Expertium]

IMO "number of new cards to add" sounds unintuitive.

[L-M-Sherlock]

There are three kinds of cards: reviewed cards, new cards which have been created and cards which will be created in the future. Should we ignore the third kind?

PS: I recommend continuing this discussion in https://forums.ankiweb.net/

[Expertium]

Ok. https://forums.ankiweb.net/t/native-implementation-of-the-simulator/46464/1

[L-M-Sherlock]

Thanks to Claude-3.5-sonnet. I have tried my best. The current simulator is OK to use. Feedback is welcome.

[Expertium]

"Additional new cards" is unclear, but idk how to make it better. A button to import all of these from a specific deck/preset would be nice.
What is group 1, group 2 and group 3?
And CMRR still needs to be available, one way or another.

[L-M-Sherlock]

A button to import all of these from a specific deck/preset would be nice.

I guess a button to apply the simulation config to the deck config is better, because the simulation config's default values are from deck config. After the user tunes the simulation config, they will want to apply it to deck config.

What is group 1, group 2 and group 3?

Group N means the N-th simulation result.

And CMRR still needs to be available, one way or another.

Of course, it's still in its original place.

[Expertium]

Maybe CMRR should be moved into the simulator? It will be more accurate that way, since it will use real limits, real deck size, etc.
Speaking of which, maybe remove "Additional new cards" and replace it with "Deck size", which the simulator will get from the real number of cards in a deck?

[L-M-Sherlock]

Maybe CMRR should be moved into the simulator? It will be more accurate that way, since it will use real limits, real deck size, etc.

No. I want to keep it as simple as possible. Just leave the complexity in simulator.

Speaking of which, maybe remove "Additional new cards" and replace it with "Deck size", which the simulator will get from the real number of cards in a deck?

It has considered the real cards, including cards in reviewing, learning and new stage. "Additional new cards" is the cards user plans to add in the future.

[user1823]

Some suggestions:

1. Rename "Additional new cards" to "Additional new cards to stimulate". Though "to stimulate" might feel superfluous, I think that it makes the function of the option much clearer.
2. The default value of "Additional new cards to stimulate" should be 0.
3. Rename "Group N" to "Simulation N".
4. Instead of using actual dates in the graph, shouldn't we use 0, 10, 20, 30, etc. (no. of days from today)?

I guess a button to apply the simulation config to the deck config is better, because the simulation config's default values are from deck config. After the user tunes the simulation config, they will want to apply it to deck config.

Seems to be a good idea.

Some more requests (if you have the time & motivation):

1. Add the options to set the Learning steps and the Relearning steps so that the user can check the effect of different steps (for e.g. multiple steps will increase the daily load when the user is doing new cards daily).
2. Add the options to enter the FSRS parameters and the desired retention
3. Add an option to plot workload vs desired retention graph. You can use this title: “Plot a graph showing workload against desired retention (slow)”

[Expertium]

1-4: agree on all four.

Next suggestions

1. Since FSRS-5 doesn't take interval lengths int account, 1m 5m and 1m 1h will have the same effect. Though, users who use many steps, like 1m 15m 2h would, indeed, see a difference.
2. Agree
3. Very computationally expensive for large decks, so idk

[L-M-Sherlock]

4. Instead of using actual dates in the graph, shouldn't we use 0, 10, 20, 30, etc. (no. of days from today)?

It's convenient for me to display days, but it's convenient for user who has a deadline date. It's easy to calculate days from date for average users, but the inverse calculation is uneasy. And the simulator add-on also displays the date:

Add the options to set the Learning steps and the Relearning steps

The simulator cannot adapt to new steps immediately if the user changes them.

Add the options to enter the FSRS parameters and the desired retention

They are in the same section FSRS, so I think it's unnecessary. If the simulator is moved to a new page, I will add these option.

Add an option to plot workload vs desired retention graph.

I will consider it. But I'm afraid that it requires to update the Rust backend to speed it up.

[user1823]

it's convenient for user who has a deadline date.

Then, we should at least express the dates in a more readable form (Jan 1, etc. like the add-on?)

The simulator cannot adapt to new steps immediately if the user changes them.

Do you mean that you will need to make significant changes to the simulator code to be able to support this? Or do you mean that there is a fundamental problem in supporting this?

I assume the former. In that case, we can keep this feature for a future version.

Add the options to enter the FSRS parameters and the desired retention

They are in the same section FSRS, so I think it's unnecessary.

But when using the simulator, the user will want to adjust these values and we don't want the user to change the actual settings and then forget to revert them to their original values.

So, I think that the simulator should have its own settings.

[L-M-Sherlock]

Then, we should at least express the dates in a more readable form (Jan 1, etc. like the add-on?)

I think it's a localization issue. yyyy-mm-dd is more readable in my local than Jan 1.

Or do you mean that there is a fundamental problem in supporting this?

It's a fundamental problem. The simulator cannot predict how many reps you will do during learning stage after changing the learning steps. It could only estimate it from the review logs.

But when using the simulator, the user will want to adjust these values and we don't want the user to change the actual settings and then forget to revert them to their original values.

OK. I will re-design it. I also plan to move the simulator into a single page.

[dae]

@L-M-Sherlock did you want me to review this now, or wait until you've made those changes?

[dae]

I recommend you keep it in the deck options for now, with the FSRS options page including a <FsrsSimulator> component or similar. Then later on, we can move that component from the options to a separate page pretty easily if we need to.

[L-M-Sherlock]

The simulator has been included in

component. So I guess we can begin to review this PR now.

[brishtibheja]

Is this worth it to add tooltips for this now or wait until there's a seperate page?

The current wording seems to be confusing so deck-options-esque tooltips might help some of the users. Also the manual, but that's for the future ig.

[L-M-Sherlock]

Is this worth it to add tooltips for this now or wait until there's a seperate page?

Which tooltips do you want the chart to provide? I will support them later.

[brishtibheja]

I brought it up because "Additional new cards" isn't very obvious to the avergae user. The other options I see probably wouldn't need it. Maybe a single help page for the simulator, then.

[dae]

I ran into some issues which I'll cover below, but when I got things working, it seems like a great addition. It also doesn't feel out of place on that page - is moving it to its own separate page something we actually need to do?

When I first tried this, I enabled FSRS on a collection and clicked on 'simulate', and I got this:

thread '<unnamed>' panicked at /Users/dae/.cargo/registry/src/index.crates.io-6f17d22bba15001f/fsrs-1.1.4/src/optimal_retention.rs:196:69:
called `Result::unwrap()` on an `Err` value: InvalidWeight

The weights in the UI are the default 19 (shown in grey).

I open a different collection that had FSRS already enabled, and get this when I click simulate:

thread '<unnamed>' panicked at /Users/dae/.cargo/registry/src/index.crates.io-6f17d22bba15001f/fsrs-1.1.4/src/optimal_retention.rs:113:6:
index out of bounds: the len is 0 but the index is 11

After I optimized the weights from their defaults and saved, I could then re-open the deck options and run the simulator.

Other thoughts:

• a graph is a much nicer way of viewing this info compared to the old feature
• we should show progress, or at least a "processing..." message, so the user doesn't think their click on the button didn't do anything
• additional new cards is capped at 10k. With 20 new cards/day, the graph will drop down early when the user uses a 10 year timespan. Is the cap for performance reasons? Is it well balanced against our days to simulate cap?

[L-M-Sherlock]

It also doesn't feel out of place on that page - is moving it to its own separate page something we actually need to do?

My concern is this part would make the deck option page too complex. But if it's OK, I'm not opposite to leave it here.

When I first tried this, I enabled FSRS on a collection and clicked on 'simulate', and I got this:

I forget to set a fallback to default parameters. I will fix it later.

I open a different collection that had FSRS already enabled, and get this when I click simulate

It's weird. Doesn't the $config.fsrsWeights store the weights?

• we should show progress

It requires to add a progress in simulate function of FSRS-rs crate. I will implement the feature later.

• Is the cap for performance reasons?

I just picked it arbitrarily. We can benchmark it in FSRS-rs.

[dae]

It's weird. Doesn't the $config.fsrsWeights store the weights?

They're set to [] in the collection I imported.

It requires to add a progress in simulate function of FSRS-rs crate. I will implement the feature later.

For an MVP, just a simple "processing..." or similar that is shown until the operation completes would be fine.

[L-M-Sherlock]

It takes 17s to complete the simulation:

[Expertium]

@L-M-Sherlock on a scale from 0% to 100%, how close are you to finishing the simulator?

[dae]

I assumed he's just waiting for me, and I'll be circling back to this soon.

[L-M-Sherlock]

on a scale from 0% to 100%, how close are you to finishing the simulator?

90%, I guess. I haven't considered the localization (strings for translation). And it's unclear how much work is waiting for me until I have more feedbacks.

[Expertium]

One last bit of feedback - you should make it clear that Simulator reads FSRS parameters, desired retention, and new/review limit settings from the preset.

[brishtibheja]

How though? In the tooltips? or by showing people "Getting parameters..." "Getting daily limits..." while simulator is working?

[Expertium]

The latter. But I'm not sure how to do it elegantly.

[dae]

Not important/required for this PR, but a match would be a bit more readable:

diff --git a/rslib/src/scheduler/fsrs/simulator.rs b/rslib/src/scheduler/fsrs/simulator.rs
index b41d98d76..61ea7074e 100644
--- a/rslib/src/scheduler/fsrs/simulator.rs
+++ b/rslib/src/scheduler/fsrs/simulator.rs
@@ -48,18 +48,15 @@ impl Collection {
             learn_limit: req.new_limit as usize,
             review_limit: req.review_limit as usize,
         };
-        let parameters = if req.weights.is_empty() {
-            DEFAULT_PARAMETERS.to_vec()
-        } else if req.weights.len() != 19 {
-            if req.weights.len() == 17 {
+        let parameters = match req.weights.len() {
+            19 => req.weights.to_vec(),
+            17 => {
                 let mut parameters = req.weights.to_vec();
                 parameters.extend_from_slice(&[0.0, 0.0]);
                 parameters
-            } else {
-                return Err(AnkiError::FsrsWeightsInvalid);
             }
-        } else {
-            req.weights.to_vec()
+            0 => DEFAULT_PARAMETERS.to_vec(),
+            _ => return Err(AnkiError::FsrsWeightsInvalid),
         };
         let (
             accumulated_knowledge_acquisition,

That said, I think this check would be better done in the FSRS crate in the future.

[dae]

This feature is marked as experimental and collapsed by default, so I have no compunctions about merging this in now, and doing some more polish in follow-up PRs. Minor things I noticed/thoughts I had:

• A little padding between the 'simulate' button and the graph box below would be good
• The simulation labels in the top right are truncated at the moment
• I don't think we need rush to make this translatable, as we may need to make wording tweaks
• "Review Time per day" is inconsistently capitalised
• Maybe: "Clear last simulation" → "Clear Last"

But all in all, it looks great. Thanks as always for your contributions everyone.

[salmanuc]

@dae Any rough estimate as to when this feature will be realeased in a beta or stable anki version?
Thanks.

[brishtibheja]

He has said it's very soon in another thread although we don't have a due date yet.