SiriusStarr / elm-spaced-repetition

Create spaced repetition software using several different popular algorithms.
GNU General Public License v3.0
11 stars 0 forks source link

Why are the due cards reversed? #7

Closed eniac314 closed 1 year ago

eniac314 commented 1 year ago

In SpacedRepetition.SMTwoAnki I do not understand why you use List.Extra.reverseMap at the end of getDueCardIndicesand getDueCardIndicesWithDetails. By doing this the cards most in need of review are sent to the end of the list right?

Removing it fixed a bug I had, but maybe the problem was on my side.

SiriusStarr commented 1 year ago
|> List.sortWith
    (\( _, c1 ) ( _, c2 ) -> compareDue deck.settings time c1 c2)
|> ListX.reverseMap Tuple.first

reverseMap is reversing a list that was sorted with compareDue. List.sortWith sorts from "least" to "greatest" (e.g. [ 1, 2, 3 ], so if compareDue puts "most due" cards at the end, they will appear first when reversed.

There is a test to make certain the list ends up sorted, so it should be working properly.

Obviously there may be a bug in the implementation of compareDue and in the test case as well, but glancing at it quickly nothing jumped out at me.

Remember (in case it was unclear) that "new" (never reviewed) cards should appear last, since the algorithm wants you to review all due cards first before adding anything new.

eniac314 commented 1 year ago

Ok I can understand never reviewed cards appearing least. I might still be doing something wrong but I noticed 2 things that seemed odd.

  1. if I build a new deck from a list of items ABCD (all new cards then) D will be first presented for review, not A. (I build the deck from the end to mitigate this)
  2. when the deck is all new, it seems that cards reviewed once (in the learning queue) then appear in last reviewed first order. That is, from a ABCD decks of news cards, if I review first AB, then stop, next time I study I 'll be presented with BACD (Learning, Learning, New, New). I was expecting ABCD.
SiriusStarr commented 1 year ago

Got it, no you're not doing anything wrong. So currently there is no specified "tie-breaking" of equivalently-scheduled cards (nor is the sort explicitly stable). This is largely irrelevant for cards that are not New or Learning, since intervals are "fuzzed" to have some randomness and thus equivalently answering two cards will result in slightly different intervals, but it shows up like this for New and Learning cards, since their intervals are fixed (or non-existent) and are not fuzzed.

So the default behavior ends up often being inverting the original order of cards with equivalent duration, if the sort ends up being stable (due to the reverseMap).

This isn't necessarily "wrong", but it's definitely not ideal, since we would like to have stronger guarantees about the order. It's an easy fix, but it's not clear to me which of the following represents the better fix:

  1. When two or more cards have equivalent intervals/"due"-ness, their ordering in the original list is preserved.
  2. When two or more cards have equivalent intervals/"due"-ness, their ordering is explicitly shuffled at random.

The latter is perhaps preferable for an SRS algorithm? The former of course allows you to implement the latter if you desire that behavior by shuffling the input, though, so is more flexible (if more manual). I am not sure which behavior Anki actually uses, as it is not a specified part of the SRS algorithm.

eniac314 commented 1 year ago

So lets say I had A New B New C New D New

If I go though the deck in the order A -> B -> C I should get a new deck in this state right? ( A Learning (due in 50 secs) B Learning (due in 53 secs) C Learning (due in 55 secs) D New

The next time around I get CBAD, couldn't the order of the cards in Learning be determined by next due first? It would seem more intuitive to me.

Would this correspond to your fix 1?

SiriusStarr commented 1 year ago

couldn't the order of the cards in Learning be determined by next due first?

The order is determined by next due first (or at the very least, should be, barring some bug that has snuck in). I think what you may be running into is that due amounts are only calculated in minutes, not seconds, so in your example, A, B, and C are all equally due.

Basically, the algorithm sees:

A Learning (due in 1 min) B Learning (due in 1 min) C Learning (due in 1 min) D New

and views A, B, and C as equivalently due, even though they were reviewed a few seconds apart.

If it is easy, would you mind waiting >60 seconds between reviews of them and see if they then appear in the expected order? (This isn't a "fix", I just want to make certain that this is why it's happening before we address it.)

eniac314 commented 1 year ago

I checked, if I wait more than 60 seconds per card the cards then appears in the order I was expecting them too.

Is there a reasons due amounts are not computed in seconds? I am doing vocabulary flashcards and most of the time only a few seconds are spent per card.

SiriusStarr commented 1 year ago

I checked, if I wait more than 60 seconds per card the cards then appears in the order I was expecting them too.

Okay, good, so there aren't any bugs, per se, outside of the ordering of equivalently due cards.

Is there a reasons due amounts are not computed in seconds?

I honestly could not tell you, since I wrote it like 4 years ago. I don't remember if I checked the Anki source and that was the way it did it or something else. My guess is just because the intervals themselves are in units of minutes, so that's the units of overdue-ness. I do think that it perhaps is better from an SRS perspective to not try to interpret "due"-ness on such short timescales, but at the same time, even a minute is too short of a timescale for that, probably.

Basically, there are two decisions that need to be made to fix this.

The downside to increasing resolution to seconds is that you will see the same cards in the same order the whole time while in the learning stages (before fuzzing kicks in), versus say a random presentation order.

Personally, I am leaning towards leaving the resolution at the scale of minutes, but shuffling the presentation order of equally due cards, so if you pass in ABCD you might see CABD or BADC or whatever, and if you review ABC in rapid succession, not D, then a minute later when they're due again you might get BAC, then D, so you don't get any "burn-in" of the ordering (and it's maybe a little less rote and boring).

What do you think of that behavior, or do you have ideas I'm overlooking?

eniac314 commented 1 year ago

Shuffling the presentation order sounds good to me then.

Thanks a lot for your all you explanations and for writing the library in the the first place!

SiriusStarr commented 1 year ago

Okay, I've implemented that behavior. Pushing a new version is going to have to wait until tomorrow, as I (foolishly) didn't make a clean branch for v3 changes, so I'm going to have to do some cherry-picking to get a non-breaking release out.

Please let me know if you encounter any other issues or pain points or have thoughts/wishlist items for #9, which is going to do a rewrite to a higher-lever API that handles more of deck management and such rather than just the base-level SRS algorithms.

eniac314 commented 1 year ago

Ok thank you for implementing the change.

As for the deck level management features, so far I have been able to make what I needed with the original API.

In the app I am making I have a function collecting all due cards from all the decks in the the model, in order to do a general review. During the general review once a card is seen I then update the corresponding original deck.

What use cases did you had in mind for combining decks?

SiriusStarr commented 1 year ago

v2.1.0 is out with the fixes, so I'm closing this issue.

In the app I am making I have a function collecting all due cards from all the decks in the the model, in order to do a general review. During the general review once a card is seen I then update the corresponding original deck.

This is the sort of stuff that I'd like the package to handle automagically in v3. Basically, allow you to have a collection of decks, get due cards for all of them, study all due cards, or only a subset of the deck, etc. Just so there's less boilerplate to be written for most usecases. (Obviously, the barebones SRS stuff will still be exposed to avoid breaking extant code.)

I'd also like to support more "advanced" things like burying and filtering cards, review limits, etc..