Make simulate iterate by card instead of by day. #235

Luc-Mcgrady · 2024-10-10T17:14:07Z

Simulating the reviews iterating card by card instead of day by day speeds up the calculation (especially with a long learn_span) and simplifies the code.
Also fixes the fact that there's a drop-off near the start.

I hope that this functions in a way pretty much exactly like the day by day version, I'll leave the tests failing so you can see exactly how the results changed.

thread 'optimal_retention::tests::simulate_with_learn_review_limit' panicked at src/optimal_retention.rs:909:9:
assertion `left == right` failed
  left: [28, 21, 40, 80, 72, 81, 86, 102, 72, 93, 105, 115, 120, 118, 124, 126, 137, 136, 158, 150, 159, 171, 156, 170, 183, 158, 195, 173, 194, 178]
 right: [0, 15, 16, 27, 69, 66, 81, 83, 75, 85, 90, 102, 107, 105, 122, 120, 141, 117, 132, 153, 153, 154, 152, 152, 149, 157, 166, 165, 184, 174]

thread 'optimal_retention::tests::simulator' panicked at src/optimal_retention.rs:864:9:
assertion `left == right` failed
  left: 7778.458
 right: 6521.068

thread 'optimal_retention::tests::optimal_retention' panicked at src/optimal_retention.rs:976:9:
assertion `left == right` failed
  left: 0.86305356
 right: 0.8277919

(/\ I've probably screwed up somewhere)

thread 'optimal_retention::tests::optimal_retention_with_old_parameters' panicked at src/optimal_retention.rs:996:9:
assertion `left == right` failed
  left: 0.8716841
 right: 0.85450846

Please note that my results are left shifted one value compared to before because "today" will have a very small cost if the user has already done their reviews. (I think).

And here's how the graphs look with my decks:

Large deck (Done reviews)

This pr

Before

(Starting drop-off lasts more than one day so not just the fact reviews are done, if I had to guess the first review of a card isn't registered for some reason)

Smaller deck (Undone reviews)

This pr

Before

Smaller deck (Done reviews)

This pr

Before

…fsrs-optimizer-burn into alternate-simulator

Fix: no learn cost

also better day_index

L-M-Sherlock · 2024-10-11T01:44:13Z

It's a good idea to make simulate iterate by card. However, it also makes it impossible to introduce any review sort order into simulation, doesn't it?

restored today on graph

Luc-Mcgrady · 2024-10-11T14:49:23Z

Good point. Hopefully by using a priority queue and iterating through the cards in the order that they are reviewed, changing the review priorities will be possible.

L-M-Sherlock · 2024-10-12T03:52:29Z

It's impossible that the number of reviews in the first day is larger than zero without any existing cards.

---- optimal_retention::tests::simulate_with_learn_review_limit stdout ----
thread 'optimal_retention::tests::simulate_with_learn_review_limit' panicked at src/optimal_retention.rs:931:9:
assertion `left == right` failed
  left: [15, 28, 28, 45, 91, 86, 109, 99, 105, 101, 112, 121, 126, 116, 129, 144, 162, 145, 140, 168, 158, 174, 166, 167, 172, 174, 172, 197, 195, 194]
 right: [0, 15, 16, 27, 69, 66, 81, 83, 75, 85, 90, 102, 107, 105, 122, 120, 141, 117, 132, 153, 153, 154, 152, 152, 149, 157, 166, 165, 184, 174]

L-M-Sherlock · 2024-10-12T03:59:49Z

src/optimal_retention.rs

+            let offset = first_rating_offsets[rating - 1];
+            let stability =
+                stability_short_term(w, w[rating - 1], offset, first_session_lens[rating - 1]);
+            let day = i / learn_limit;


learn_limit is usize::MAX when max_cost_perday is enable. The code doesn't take it into account.

I think this was fixed by this now that learn cards are reviewed in the main loop.

fsrs-rs/src/optimal_retention.rs

Lines 228 to 235 in 59798eb

if (review_cnt_per_day[day_index] + 1 > review_limit)

|| (!not_learn && learn_cnt_per_day[day_index] + 1 > learn_limit)

|| (cost_per_day[day_index] + fail_cost > max_cost_perday)

{

card.due += 1.;

card_priorities.change_priority(&card_index, card_priority(card, !not_learn));

continue;

}

Cards will all still be on day 0 in the queue but will be pushed until days which low cost enough.

L-M-Sherlock · 2024-10-12T04:01:26Z

src/optimal_retention.rs

+    for (i, card) in cards.iter().enumerate() {
+        card_priorities.push(i, card_priority(card));
+    }


If the card hasn't been learned, it shouldn't be pushed here, right?

src/optimal_retention.rs

L-M-Sherlock · 2024-10-13T06:07:58Z

src/optimal_retention.rs

+        let retrievability = power_forgetting_curve(delta_t, card.stability);

        // Create 'forget' mask
-        let forget = Zip::from(&rand_slice)
-            .and(&retrievability)
-            .map_collect(|&rand_val, &retriev_val| rand_val > retriev_val);
+        let forget = !rng.gen_bool(retrievability as f64);

        // Sample 'rating' for 'need_review' entries
-        let mut ratings = Array1::zeros(deck_size);
-        izip!(&mut ratings, &need_review, &forget)
-            .filter(|(_, &condition, _)| condition)
-            .for_each(|(rating, _, forget)| {
-                *rating = if *forget {
-                    1
-                } else {
-                    review_rating_choices[review_rating_dist.sample(&mut rng)]
-                };
-            });
-
-        // Update 'cost' column based on 'need_review', 'forget' and 'ratings'
-        izip!(&mut cost, &need_review, &forget, &ratings)
-            .filter(|(_, &need_review_flag, _, _)| need_review_flag)
-            .for_each(|(cost, _, &forget_flag, &rating)| {
-                *cost = if forget_flag {
-                    review_costs[0] * loss_aversion
-                } else {
-                    review_costs[rating - 1]
-                }
-            });
+        let rating = if forget {
+            1
+        } else {
+            review_rating_choices[review_rating_dist.sample(&mut rng)]
+        };
+
+        //dbg!(&card, &rating);

-        // Calculate cumulative sum of 'cost'
-        let mut cum_sum = Array1::<f32>::zeros(deck_size);
-        cum_sum[0] = cost[0];
-        for i in 1..deck_size {
-            cum_sum[i] = cum_sum[i - 1] + cost[i];
+        // Update 'cost' based on 'forget' and 'rating'
+        let cost = if not_learn {
+            if forget {
+                review_costs[0] * loss_aversion
+            } else {
+                review_costs[rating - 1]
+            }
+        } else {
+            learn_costs[rating - 1]
+        };
+
+        // Wait until a day which is available
+        while day_index < learn_span && cost_per_day[day_index] + cost > max_cost_perday {
+            day_index += 1;
+        }
+        if day_index >= learn_span {
+            card_priorities.pop();
+            continue;
        }

-        // Create 'true_review' mask based on 'need_review' and 'cum_sum' and 'review_limit'
-        let mut review_count = 0;
-        let true_review =
-            Zip::from(&need_review)
-                .and(&cum_sum)
-                .map_collect(|&need_review_flag, &cum_cost| {
-                    if need_review_flag {
-                        review_count += 1;
-                    }
-                    need_review_flag
-                        && (cum_cost <= max_cost_perday)
-                        && (review_count <= review_limit)
-                });
-
-        let need_learn = old_due.mapv(|x| x == learn_span as f32);
-        // Update 'cost' column based on 'need_learn'
-        izip!(&mut cost, &need_learn, &init_ratings)
-            .filter(|(_, &need_learn_flag, _)| need_learn_flag)
-            .for_each(|(cost, _, &rating)| {
-                *cost = learn_costs[rating - 1];
-            });
-
-        cum_sum[0] = cost[0];
-        for i in 1..deck_size {
-            cum_sum[i] = cum_sum[i - 1] + cost[i];
+        let retrievability =
+            power_forgetting_curve(day_index as f32 - card.last_date, card.stability);


The rating is sampled from the first retrievability. However, the second retrievability is the real one.

I ran into problems with this because

The rating depends on the retrievability

The cost depends on the rating

The cost determines if the card has a cost too high to fit in the day (it might change the day)

The day determines the retrievability

Would it be ok to just assume the card is forgotten when checking whether the cost is too much for that day or not? Something like:

while day_index < learn_span && cost_per_day[day_index] + (review_costs[0] * loss_aversion) // Card hasn't been reviewed yet, we are assuming they forgot. > max_cost_perday { day_index += 1; }

To avoid rating re-roll shenanigans.

It isn't OK to assume that. For example, assuming the original interval is 365 days, a delay of one day doesn't have an significant impact. But if the original interval is 1 day, the delay will induce a large impact.

Oops. I'm sorry I misunderstood. It's OK to use the maximum cost when check whether the cost is too much for that day.

I think fixing it will solve this test:

thread 'optimal_retention::tests::simulator' panicked at src/optimal_retention.rs:898:9:
assertion left == right failed
left: 8334.49
right: 6521.068

In the implementation of current PR, the memorization is significantly higher than the previous on. Because it's too optimistic to sample the rating from the first retrievability.

With the change:

left: 5832.3096
right: 6521.068

I'm assuming the memorisation has probably gone under because we're assuming the card failed which the current version doesn't do?

At least the value is closer to the expected value than the previous one.

I guess it's because sometime the forget_cost is so large that the review is delayed too much, which decreases the memorization.

src/optimal_retention.rs

L-M-Sherlock · 2024-10-14T10:51:33Z

src/optimal_retention.rs

+        let upper = min(day_index + ivl as usize, learn_span);
+        for i in day_index..upper {
+            memorized_cnt_per_day[i] += retrievability;
+        }


I figure it out. The retrievability should decay over time.

left: 6475.4844
right: 6521.068

Oh yeah that's a lot better.

L-M-Sherlock · 2024-10-14T11:20:48Z

---- optimal_retention::tests::simulate_with_learn_review_limit stdout ----

This test still has a large difference. I will dive into it later.

Luc-Mcgrady · 2024-10-14T12:41:43Z

I think that's better? I think it was treating new cards as review cards at certain points.

 left: [0, 13, 23, 30, 58, 81, 83, 81, 86, 88, 88, 106, 120, 110, 122, 133, 132, 121, 131, 143, 161, 188, 142, 179, 145, 156, 172, 191, 174, 165]
 right: [0, 15, 16, 27, 69, 66, 81, 83, 75, 85, 90, 102, 107, 105, 122, 120, 141, 117, 132, 153, 153, 154, 152, 152, 149, 157, 166, 165, 184, 174]

L-M-Sherlock · 2024-10-15T06:09:55Z

Simulating the reviews iterating card by card instead of day by day speeds up the calculation (especially with a long learn_span) and simplifies the code.

Could you benchmark it?

Luc-Mcgrady · 2024-10-15T12:32:30Z

Well on my computer at least:

Current:

optimal_retention       time:   [274.85 ms 278.54 ms 282.14 ms]

This pr:

optimal_retention       time:   [35.805 ms 36.197 ms 36.589 ms]
                        change: [-87.228% -87.005% -86.782%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Expertium · 2024-10-15T13:29:40Z

Dang, that's big

L-M-Sherlock · 2024-10-15T14:17:14Z

[src\optimal_retention.rs:505:9] iter = 5
thread 'optimal_retention::tests::optimal_retention_with_old_parameters' panicked at src\optimal_retention.rs:1047:9:
assertion `left == right` failed
  left: 0.82624805
 right: 0.85450846

This difference is still larger than my expectation. I plan to do a T-test tomorrow to check whether the difference is statistically significant. If not, I will merge it.

L-M-Sherlock · 2024-10-16T03:05:18Z

After increasing the sample_size, the results are close enough. I will test it in Anki for final resolution.

L-M-Sherlock

LGTM

asukaminato0721 · 2024-10-16T05:26:10Z

~~just want to say that rust has built-in priority queue https://doc.rust-lang.org/std/collections/binary_heap/index.html~~ , but currently this works well, so let it be.

Luc-Mcgrady added 18 commits October 9, 2024 15:57

Alternate simulator

3b3d4d0

clippy

f5a5a76

Fix: Learn limit

5724b44

Merge branch 'open-spaced-repetition:main' into alternate-simulator

d7c7c05

Fix: Typo

d230051

Merge branch 'alternate-simulator' of https://github.com/Luc-Mcgrady/…

9b5a97c

…fsrs-optimizer-burn into alternate-simulator

Fix: Last day not simulated

3285828

Removed: Un-needed clamp

18202a6

Fix: Non cost stats

5077a2e

Fix: no learn cost

Fix: Learn timings

40ccc57

Fix: First day doubled

7067d61

Fix: Double reviews on first day

4914227

Fix: Spike at start

fef994e

Fix: memorized_cnt_per_day

9f2f8ef

also better day_index

fmt

e5bcab3

Fix: offset issues

6489faf

fmt

abeb208

Fix: memorized_cnt_per_day of learn_card

fccef27

Luc-Mcgrady added 4 commits October 11, 2024 14:41

Added: Priority queue

eb23616

Simplified card priority

2e67342

Fix: Day overlap issues

5166d98

restored today on graph

Fix: retrievability not recalculated

ef88f64

L-M-Sherlock reviewed Oct 12, 2024

View reviewed changes

Luc-Mcgrady added 4 commits October 12, 2024 18:01

Fix: Learn card issues

aab43bb

Added: Learn limit test

e8ea727

Fix: New card pooling

edb971f

fmt

87bd7f3

Removed dbg

a79f422

L-M-Sherlock reviewed Oct 13, 2024

View reviewed changes

fix review_limit

cbe8f9d

Luc-Mcgrady commented Oct 13, 2024

View reviewed changes

src/optimal_retention.rs Outdated Show resolved Hide resolved

Max cost per day based on fail

59798eb

L-M-Sherlock reviewed Oct 14, 2024

View reviewed changes

update retrievability when calculating memorized_cnt_per_day

fd233ca

Moved learn cards into main loop

4601d2d

Luc-Mcgrady and others added 3 commits October 14, 2024 15:10

Fix: incorrectly marked learn

5f08ffe

cargo fmt

4f9f594

cargo clippy

a80c892

Luc-Mcgrady and others added 3 commits October 15, 2024 21:52

Test: Simulator costs

6c18029

increase sample_size && update test results

8c0a2c1

bump version

ab241dc

apply max_ivl after next_interval

1fd96b6

L-M-Sherlock marked this pull request as ready for review October 16, 2024 03:33

L-M-Sherlock approved these changes Oct 16, 2024

View reviewed changes

L-M-Sherlock requested a review from asukaminato0721 October 16, 2024 03:40

asukaminato0721 approved these changes Oct 16, 2024

View reviewed changes

asukaminato0721 merged commit 849dfa7 into open-spaced-repetition:main Oct 16, 2024
3 checks passed

L-M-Sherlock mentioned this pull request Oct 18, 2024

update FSRS-rs to 1.3.3 open-spaced-repetition/fsrs-browser#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make simulate iterate by card instead of by day. #235

Make simulate iterate by card instead of by day. #235

Luc-Mcgrady commented Oct 10, 2024 •

edited

Loading

L-M-Sherlock commented Oct 11, 2024

Luc-Mcgrady commented Oct 11, 2024

L-M-Sherlock commented Oct 12, 2024

L-M-Sherlock Oct 12, 2024

Luc-Mcgrady Oct 14, 2024

L-M-Sherlock Oct 12, 2024

L-M-Sherlock Oct 13, 2024

Luc-Mcgrady Oct 13, 2024

L-M-Sherlock Oct 13, 2024

L-M-Sherlock Oct 14, 2024

L-M-Sherlock Oct 14, 2024

Luc-Mcgrady Oct 14, 2024

L-M-Sherlock Oct 14, 2024 •

edited

Loading

L-M-Sherlock Oct 14, 2024

L-M-Sherlock Oct 14, 2024

Luc-Mcgrady Oct 14, 2024

L-M-Sherlock commented Oct 14, 2024

Luc-Mcgrady commented Oct 14, 2024

L-M-Sherlock commented Oct 15, 2024

Luc-Mcgrady commented Oct 15, 2024

Expertium commented Oct 15, 2024

L-M-Sherlock commented Oct 15, 2024

L-M-Sherlock commented Oct 16, 2024

L-M-Sherlock left a comment

asukaminato0721 commented Oct 16, 2024

	if (review_cnt_per_day[day_index] + 1 > review_limit)
	\|\| (!not_learn && learn_cnt_per_day[day_index] + 1 > learn_limit)
	\|\| (cost_per_day[day_index] + fail_cost > max_cost_perday)
	{
	card.due += 1.;
	card_priorities.change_priority(&card_index, card_priority(card, !not_learn));
	continue;
	}

Make simulate iterate by card instead of by day. #235

Make simulate iterate by card instead of by day. #235

Conversation

Luc-Mcgrady commented Oct 10, 2024 • edited Loading

Large deck (Done reviews)

This pr

Before

Smaller deck (Undone reviews)

This pr

Before

Smaller deck (Done reviews)

This pr

Before

L-M-Sherlock commented Oct 11, 2024

Luc-Mcgrady commented Oct 11, 2024

L-M-Sherlock commented Oct 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-M-Sherlock Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-M-Sherlock commented Oct 14, 2024

Luc-Mcgrady commented Oct 14, 2024

L-M-Sherlock commented Oct 15, 2024

Luc-Mcgrady commented Oct 15, 2024

Expertium commented Oct 15, 2024

L-M-Sherlock commented Oct 15, 2024

L-M-Sherlock commented Oct 16, 2024

L-M-Sherlock left a comment

Choose a reason for hiding this comment

asukaminato0721 commented Oct 16, 2024

Luc-Mcgrady commented Oct 10, 2024 •

edited

Loading

L-M-Sherlock Oct 14, 2024 •

edited

Loading