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

Fix/wrong calc elapsed days #144

Merged
merged 3 commits into from
Jan 5, 2025
Merged

Fix/wrong calc elapsed days #144

merged 3 commits into from
Jan 5, 2025

Conversation

ishiko732
Copy link
Collaborator

@ishiko732 ishiko732 requested a review from L-M-Sherlock January 4, 2025 10:16
Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (af8f873) to head (848875c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #144   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          689       693    +4     
  Branches        76        76           
=========================================
+ Hits           689       693    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ishiko732 ishiko732 added the bug Something isn't working label Jan 4, 2025
@ishiko732 ishiko732 requested a review from Luc-Mcgrady January 4, 2025 10:36
@Luc-Mcgrady
Copy link
Member

Before:
image
Now:
image

https://github.com/Luc-Mcgrady/Anki-Search-Stats-Extended/blob/line-graph/src/ts/MemorisedBar.ts
Provided my code works as intended, there are still cards which have their stabilities different between running it with this and Anki.
Could it be Anki's rollover?

@L-M-Sherlock
Copy link
Member

Could it be Anki's rollover?

Could you provide a unit test to reproduce the inconsistency? Like this:

test('TS-FSRS-Simulator', () => {
    const f = fsrs({
        w: [1.1596,1.7974,13.1205,49.3729,7.2303,0.5081,1.5371,0.001,1.5052,0.1261,0.9735,1.8924,0.1486,0.2407,2.1937,0.1518,3.0699,0.4636,0.6048],
    })
    const rids = [1704468957000, 1704469645000, 1704599572000, 1705509507000]
    let card = createEmptyCard(new Date(rids[0]))
    const grades : Grade[] = [Rating.Good, Rating.Good, Rating.Good, Rating.Good]
    for (let i = 0; i < rids.length; i++) {
        const now = new Date(rids[i])
        const log = f.next(card, now, grades[i])
        card = log.card
        console.log(card.stability)
    }
  })

@ishiko732
Copy link
Collaborator Author

Provided my code works as intended, there are still cards which have their stabilities different between running it with this and Anki.
Could it be Anki's rollover?

Since I cannot directly run the Anki-Search-Stats-Extended code, I'm not sure if there's an issue with it; perhaps the last scheduling in historical cards was not using FSRS-5, but FSRS-4 or FSRS-4.5, which might explain the inconsistency in calculated stability?

@ishiko732
Copy link
Collaborator Author

some Anki card records have s values based on FSRS-4.5.

image

select *
from revlog
where cid= 1631872042742
order by id desc

#,id,cid,usn,ease,ivl,lastIvl,factor,time,type
1,1714744568062,1631872042742,612,3,74,31,861,60000,1
2,1711352380923,1631872042742,327,2,36,24,890,60000,1
3,1709171131116,1631872042742,150,3,20,10,815,7451,1
4,1708334378635,1631872042742,109,3,6,3,827,554,1
5,1708102403848,1631872042742,101,2,3,2,841,6217,1
6,1707902884484,1631872042742,93,2,2,2,763,46325,1
7,1707640884786,1631872042742,84,4,2,-60,682,3651,0
8,1707640515087,1631872042742,84,2,-330,0,682,27105,0

select *
from cards
where id= 1631872042742

id,nid,did,ord,mod,usn,type,queue,due,ivl,factor,reps,lapses,left,odue,odid,flags,data
1631872042742,1631872042742,1707630005524,0,1714744568,612,2,2,303,74,2050,8,0,0,0,0,0,"{""pos"":81,""s"":68.49,""d"":7.846,""dr"":0.9,""cd"":""{""v"":""v4.11.1"",""seed"":7621,""d"":7.1,""s"":70.85}""}"


ts-fsrs v4.5.2 using FSRS-5

test('TS-FSRS-Simulator[ts-fsrs v4.5.2 FSRS-5]', () => {
  const f = fsrs({
    w: [
      1.0139, 1.0139, 4.6959, 11.3189, 5.0903, 1.121, 0.8626, 0.0606, 1.628,
      0.1, 1.0006, 2.1484, 0.0825, 0.3375, 1.4004, 0.1114, 2.7801,
    ],
  })
  const rids = [
    1707640515087, 1707640884786, 1707902884484, 1708102403848, 1708334378635,
    1709171131116, 1711352380923, 1714744568062,
  ]
  const expected = [
    1.0139, 1.0139, 1.75389969, 2.25526707, 8.22141138, 26.63123583,
    31.49567267, 91.53053746,
  ]
  let card = createEmptyCard(new Date(rids[0]))
  const grades: Grade[] = [2, 4, 2, 2, 3, 3, 2, 3]
  for (let i = 0; i < rids.length; i++) {
    const now = new Date(rids[i])
    const log = f.next(card, now, grades[i])
    card = log.card
    expect(card.stability).toBeCloseTo(expected[i], 4)
  }
// card.stability => 91.53
})

ts-fsrs v3.5.7(patch elapsed_days) using FSRS-4.5

git checkout v3.5.7
pnpm install

After applying the modifications from this PR to the src/fsrs/scheduler.ts#SchedulingCard.constructor related code, proceed with testing.

    card.elapsed_days =
-      card.state === State.New ? 0 : now.diff(card.last_review as Date, "days");
    card.elapsed_days =
+      card.state === State.New ? 0 : dateDiffInDays(this.last_review, now);
test("TS-FSRS-Simulator[ts-fsrs v3.5.7(patch elapsed_days) FSRS-v4.5]", () => {
  const f = fsrs({
    w: [
      1.0139, 1.0139, 4.6959, 11.3189, 5.0903, 1.121, 0.8626, 0.0606, 1.628,
      0.1, 1.0006, 2.1484, 0.0825, 0.3375, 1.4004, 0.1114, 2.7801,
    ],
  });
  const rids = [
    1707640515087, 1707640884786, 1707902884484, 1708102403848, 1708334378635,
    1709171131116, 1711352380923, 1714744568062,
  ];
  const expected = [
    1.0139, 1.0139, 1.7317146, 2.18208164, 6.98174642, 21.80173316, 25.74674998,
    68.49768733,
  ];
  const col: number[] = [];
  let card = createEmptyCard(new Date(rids[0]));
  const grades: Grade[] = [2, 4, 2, 2, 3, 3, 2, 3];
  for (let i = 0; i < rids.length; i++) {
    const now = new Date(rids[i]);
    const log = f.repeat(card, now)[grades[i]];
    card = log.card;
    col.push(card.stability);
    expect(card.stability).toBeCloseTo(expected[i], 4);
  }
  console.log(card.stability); // card.stability => 68.49
});

Copy link
Member

@Luc-Mcgrady Luc-Mcgrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow thanks for all the effort!

Reverting to v3.5.7 didn't work for me.
image

v3.5.7+fix:
image
This pr:
image

const f = fsrs({
    w: [2.3087, 4.1277, 9.8679, 17.6430, 7.2056, 0.3775, 1.7137, 0.0011, 1.4670, 0.1637, 0.9486, 1.9157, 0.1287, 0.3025, 2.3155, 0.0001, 2.8344, 0.3571, 0.6384]
  })
const rids = [1665423581318,1665423653089,1665424320968,1665433930433,1665480844915,1665749731632,1666458872005,1667929850092,1671571024108,1680131629095,1689548964502]
const grades: Grade[] = [1,3,3,3,3,3,3,3,3,2,3]

I'd say that's close enough for the graph.


Here's an example problem card, though I expect this is out of the scope of this pull request.

image

id cid usn ease ivl lastIvl factor time type
1735993129140 1662942994437 -1 0 882 25 589 0 5
1713000017248 1662942994437 18391 3 624 119 589 6207 1
1704910583686 1662942994437 16287 0 143 143 590 0 4
1702718934003 1662942994437 15532 3 102 47 565 2668 1
1699260169343 1662942994437 14468 0 20 18 550 0 4
1698698192380 1662942994437 14275 3 18 -7200 2500 4235 0
1698688916440 1662942994437 14271 3 -7200 -600 0 1326 0
1698688837021 1662942994437 14271 3 -600 -60 0 1363 0
1698688771401 1662942994437 14271 1 -60 -7200 0 6472 0
1698678126399 1662942994437 14261 3 -7200 -600 0 1965 0
1698678054940 1662942994437 14260 3 -600 0 0 10617 0
first_of_last_revlog_entries ^ so I truncate it here
1697182240703 1662942994437 13788 0 0 139 0 0 4
1688803646964 1662942994437 9846 3 100 70 2450 3962 1
1682408696385 1662942994437 7589 3 129 53 2450 9448 1
1677870162608 1662942994437 5711 3 53 27 2450 21773 1
1673944811781 1662942994437 4498 3 102 42 2450 8779 1
1670319241213 1662942994437 3546 4 42 14 2450 4508 1
1669104703673 1662942994437 3216 3 14 6 2300 18944 1
1668610746240 1662942994437 3031 3 6 -86400 2300 4327 2
1668507588958 1662942994437 2985 3 -86400 -600 2300 3370 2
1668506742137 1662942994437 2984 3 -600 -60 2300 5112 2
1668506506324 1662942994437 2983 1 -60 -86400 2300 30000 2
1668429286472 1662942994437 2946 3 -86400 -600 2300 9298 2
1668427317488 1662942994437 2943 3 -600 -60 2300 3325 2
1668427235731 1662942994437 2941 1 -60 27 2300 12559 1
1666095772446 1662942994437 1738 3 27 11 2500 4827 1
1665140780850 1662942994437 1106 3 11 5 2500 7384 1
1664707697911 1662942994437 705 3 5 3 2500 20502 1
1664439011033 1662942994437 563 3 3 1 2500 4657 1
1664392046978 1662942994437 548 3 1 -7200 2500 12497 0
1664382514443 1662942994437 542 3 -7200 -600 0 5423 0
1664382303688 1662942994437 540 3 -600 -60 0 4676 0
1664382285359 1662942994437 540 1 -60 -7200 0 8144 0
1664375482797 1662942994437 533 3 -7200 -600 0 17122 0
1664371293852 1662942994437 528 3 -600 -60 0 6710 0
1664371215663 1662942994437 528 1 -60 -60 0 9852 0

Card:

id nid did ord mod usn type queue due ivl factor reps lapses left odue odid flags data
1662942994437 1662942994329 1663460765931 1 1736029349 -1 2 2 684 103 2500 8 0 0 0 0 0 {"pos":7916,"s":71.77,"d":8.23,"dr":0.87,"cd":"{"v": "reschedule"}"}

v3.5.7+fix:
image

This pr:
image

Heres the card quickly pasted into FSRS-rs
image

test('SSE', () => {
  const f = fsrs({
    w: [0.4911, 4.5674, 24.8836, 77.0450, 7.5474, 0.1873, 1.7732, 0.0010, 1.1112, 0.1520, 0.5728, 1.8747, 0.1733, 0.2449, 2.2905, 0.0000, 2.9898, 0.0883, 0.9033]
  })

  const rids = [1698678054940,1698678126399,1698688771401,1698688837021,1698688916440,1698698192380,1699260169343,1702718934003,1704910583686,1713000017248]
  const ratings: Rating[] = [3,3,1,3,3,3,0,3,0,3]

  let card = createEmptyCard(new Date(rids[0]))

  for (let i = 0; i < rids.length; i++) {
    const rating = ratings[i]
    if (rating == 0) {
      continue
    }

    const now = new Date(rids[i])
    const log = f.repeat(card, now)[rating]
    card = log.card
  }


 // For pasting into ts-fsrs
  let last = new Date(rids[0]);
  let delta_t: any[] = []
  for (let i = 0; i < rids.length; i++) {
    const rating = ratings[i]
    if (rating == 0) {
      continue
    }

    const current = new Date(rids[i]);
    delta_t.push({delta_t: dateDiffInDays(last, current), rating})
    last = new Date(rids[i]);
  }
  console.log(delta_t)
  
  expect(card.stability).toBeCloseTo(71.77)
})

@L-M-Sherlock
Copy link
Member

So, is it OK to merge this PR?

Copy link
Member

@L-M-Sherlock L-M-Sherlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ishiko732
Copy link
Collaborator Author

Here's an example problem card, though I expect this is out of the scope of this pull request.

Let's discuss how to handle this special case in issue #145.

@ishiko732 ishiko732 merged commit cdd9158 into main Jan 5, 2025
5 checks passed
@ishiko732 ishiko732 deleted the Fix/wrong-calc-elapsed-days branch January 5, 2025 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants