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

Add descending retrievability #3559

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Add descending retrievability #3559

merged 2 commits into from
Nov 8, 2024

Conversation

dae
Copy link
Member

@dae dae commented Nov 7, 2024

A partial fix for #3460. Some notes:

  • This only adds the extra option for now, and does not change the default yet.
  • The 'day', 'deck then day' and 'day then deck' orders sort by day primarily because it was the most efficient in older versions of Anki, and that is no longer as much of a concern. When making the this new sort order the default in the future, we may want to drop 'day' and 'day then deck', and replace 'deck then day' with 'deck then most retrievable first'.
  • In the end, I decided not to add the special treatment of the current day for now, as I'd like to see how the simpler approach is received first.

What do you all think?

@dae dae requested a review from abdnh November 7, 2024 06:18
@user1823
Copy link
Contributor

user1823 commented Nov 7, 2024

Looks good enough to me.

Just to note, this seems to remove the Relative Overdueness option for SM-2 users.

Copy link
Collaborator

@abdnh abdnh left a comment

Choose a reason for hiding this comment

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

LGTM

@abdnh
Copy link
Collaborator

abdnh commented Nov 7, 2024

Just to note, this seems to remove the Relative Overdueness option for SM-2 users.

It's synonymous with ascending retrievability - the new order also works with SM-2.

@user1823
Copy link
Contributor

user1823 commented Nov 7, 2024

Sorry, I didn't look closely at the code.

temp_string = format!("ivl / cast({today}-due+0.001 as real)", today = today);
ReviewOrderSubclause::RetrievabilitySm2 { today, order } => {
temp_string = format!(
"ivl / cast({today}-due+0.001 as real) {order}",
Copy link
Contributor

@user1823 user1823 Nov 7, 2024

Choose a reason for hiding this comment

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

This is fine. But, for a more accurate sorting (adding 0.001 to avoid division by zero introduces slight inaccuracy), we can calculate the reciprocal and then sort in the opposite direction.

So, it would become something like cast(due-{today} as real) / ivl {order}

Copy link
Member Author

Choose a reason for hiding this comment

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

No objections to a more accurate implementation, but I'm nervous to change working code so close to a release - perhaps this could be fixed in a follow-up.

@dae dae merged commit b646f09 into main Nov 8, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants