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

Why are the due cards reversed? #7

Closed
eniac314 opened this issue Aug 27, 2023 · 11 comments
Closed

Why are the due cards reversed? #7

eniac314 opened this issue Aug 27, 2023 · 11 comments

Comments

@eniac314
Copy link

In SpacedRepetition.SMTwoAnki I do not understand why you use List.Extra.reverseMap at the end of getDueCardIndices and 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
Copy link
Owner

|> 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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

SiriusStarr commented Aug 28, 2023

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
Copy link
Author

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
Copy link
Owner

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.

  • Do we want equivalently due cards shuffled or in input order?
  • Do we want to increase the resolution of due-ness to the order of seconds, not minutes?

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants