fasiha / ebisu.js

JavaScript port of Ebisu, the public-domain library for Bayesian quiz scheduling.
The Unlicense
45 stars 7 forks source link

Small tau values cause `failed to converge` errors #20

Closed LazerJesus closed 1 year ago

LazerJesus commented 1 year ago

hey, i am working on my implementation and stumbled on a minor issue. Any tau value smaller than 0.0608 + that time cant be computed.

try {
    const tau = 0.0607;
    const time = 3.56;
    ebisu.updateModel([4, 4, tau], 1, 1, time);
} catch (e) {
    console.log(e);
}

results in error:

394 |   let et;
395 |   if (rebalance) {
396 |     const status = {};
397 |     const sol = fmin((et2) => Math.abs(moment(1, et2) - 0.5), { lowerBound: 0 }, status);
398 |     if (!("converged" in status) || !status.converged) {
399 |       throw new Error("failed to converge");
                ^
error: failed to converge
      at _updateRecallSingle (/Users/finn/valence/code/spanish2/backend/node_modules/ebisu-js/dist/ebisu.cjs:399:12)
      at updateRecall (/Users/finn/valence/code/spanish2/backend/node_modules/ebisu-js/dist/ebisu.cjs:315:11)
      at /Users/finn/valence/code/spanish2/backend/src/games/spacedrepetition/updateNode.js:76:4
      at processTicksAndRejections (:55:76)

its an issue in an upstream library minimize-golden-section-1d I THINK (dont quote me on that). anyways, no biggie for me just sending signal.

LazerJesus commented 1 year ago

happended again. tau of .16 and .26 work. .24 not. this time its actually a problem.

    const tau = 0.24;
    const time = 14.39;
    const model = [4, 4, tau];
    const update = ebisu.updateModel(model, 1, 1, time);
394 |   let et;
395 |   if (rebalance) {
396 |     const status = {};
397 |     const sol = fmin((et2) => Math.abs(moment(1, et2) - 0.5), { lowerBound: 0 }, status);
398 |     if (!("converged" in status) || !status.converged) {
399 |       throw new Error("failed to converge");
                ^
error: failed to converge
      at _updateRecallSingle (/Users/finn/valence/code/spanish2/backend/node_modules/ebisu-js/dist/ebisu.cjs:399:12)
      at updateRecall (/Users/finn/valence/code/spanish2/backend/node_modules/ebisu-js/dist/ebisu.cjs:315:11)
      at /Users/finn/valence/code/spanish2/backend/src/games/spacedrepetition/updateNode.js:102:19
      at processTicksAndRejections (:55:76)
fasiha commented 1 year ago

Thanks soooo much for reporting this and so sorry for the delay! Fixed in version 2.1.1β€”please try it out to confirm it works 😁 per the new unit tests, this shouldn't happen for much smaller tau/t ratios (the new unit test asserts that a quiz that's 100'000x the halflife will continue to work πŸ’ͺ)

LazerJesus commented 8 months ago
$ ebisu.updateRecall([ 3.9951083696258847, 3.9951083696258847, 238402764.59541383 ], 1, 1, 1.1461897222222222);

> {
  iterations: 100,
  argmin: 221614253.88549274,
  minimum: 1.2212453270876722e-15,
  converged: false
} {
  prior: [ 3.9951083696258847, 3.9951083696258847, 238402764.59541383 ],
  result: 1,
  tnow: 1.0757555555555558,
  q0: 0,
  rebalance: true,
  tback: undefined
}
Error: failed to converge
    at _updateRecallSingle (/Users/finn/vivalence/code/spanish/app/frontend/node_modules/ebisu-js/dist/ebisu.cjs:420:13)
    at _updateRecallSingle (/Users/finn/vivalence/code/spanish/app/frontend/node_modules/ebisu-js/dist/ebisu.cjs:417:16)
    at Proxy.updateRecall (/Users/finn/vivalence/code/spanish/app/frontend/node_modules/ebisu-js/dist/ebisu.cjs:318:12)

getting the same error again. this time with larger tau values.

on 2.1.2

this isnt blocking me at all, just an fyi.

fasiha commented 8 months ago

@LazerJesus thanks for this even more crazy example πŸ˜‚ I released v2.1.3 that gives you a backdoor to fix this: more details in https://github.com/fasiha/ebisu.js/pull/25 but

ebisu.updateRecall([4, 4, 1e9], 1, 1, 1.5) // πŸ’£βŒ

will throw but

ebisu.updateRecall([4, 4, 1e9], 1, 1, 1.5, undefined, undefined, undefined, {tolerance: 1e-6}) // βœ…βœ…

will now work. The default tolerance is 1e-8 and by relaxing it a bit, this extreme case converges. I decided to not change the default tolerance because I think this edge case is pretty rare but the backdoor is there if you ever need this.

One billion hours/days halflife πŸ€ͺ

LazerJesus commented 8 months ago

what can i say, i know this fact reeaaallllyyyy well πŸ˜†