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

Feat/true retention stats #3425

Merged
merged 9 commits into from
Sep 22, 2024
Merged

Conversation

L-M-Sherlock
Copy link
Contributor

@dae
Copy link
Member

dae commented Sep 20, 2024

  • If this is not expected to change soon, perhaps we should be making it translatable from the start?
  • The summary at the bottom is a bit long. Could we move it to the top, and make it simpler? E.g. 'Pass rate of cards with an interval >= 1 day'?

@dae
Copy link
Member

dae commented Sep 20, 2024

Also, the green text doesn't fit so well with the rest of Anki's interface. Maybe standard black would be a better fit, with perhaps some bolding on key elements?

@L-M-Sherlock
Copy link
Contributor Author

  • If this is not expected to change soon, perhaps we should be making it translatable from the start?

Good point! I will make it translatable later.

  • The summary at the bottom is a bit long. Could we move it to the top, and make it simpler? E.g. 'Pass rate of cards with an interval >= 1 day'?

The summary is written by @user1823 in open-spaced-repetition/fsrs4anki-helper#200. I'd like to ask for his opinion.

Also, the green text doesn't fit so well with the rest of Anki's interface. Maybe standard black would be a better fit, with perhaps some bolding on key elements?

OK.

@user1823
Copy link
Contributor

'Pass rate of cards with an interval >= 1 day'?

Looks good.

@L-M-Sherlock
Copy link
Contributor Author

image

@Expertium
Copy link
Contributor

Expertium commented Sep 21, 2024

"7 days ago" and "30 days ago" implies that it's retention for ONE specific day. "Last week" and "Last month" is better. "Last year" would be nice, though this is already a ton of numbers.

@L-M-Sherlock
Copy link
Contributor Author

OK

image

I also removed the Card Counts part because it's confusing and irrelevant to true retention.

@Expertium
Copy link
Contributor

Will you add these as well?
image
I really hope so, I like knowledge acquisition rate, and I think a lot of people will like it too

ftl/core/statistics.ftl Outdated Show resolved Hide resolved
Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com>
@L-M-Sherlock
Copy link
Contributor Author

I really hope so, I like knowledge acquisition rate, and I think a lot of people will like it too

The stats has included some of them:

image image

@Expertium
Copy link
Contributor

Some, yes, but not all

@L-M-Sherlock
Copy link
Contributor Author

Fine. I added the Estimated total knowledge (cards):

image

Daily load and retention by notes require more works to do, so I will consider it in another PR.

@Expertium
Copy link
Contributor

Expertium commented Sep 22, 2024

I think these stats need their own dedicated section, with explanations copied from the add-on.
I mean:

  1. Estimated total knowledge
  2. Knowledge acquisiton rate
  3. Daily load

@dae
Copy link
Member

dae commented Sep 22, 2024

Thanks Jarrett.

Some minor issues I noticed that I won't block on:

  • collecting the results into a hashmap first is not the most efficient, but it doesn't really matter in this case
  • total knowledge is probably more useful as an integer
  • in some cases, the start of the table is cut off as the graphs window is resized

@dae dae merged commit 3912db3 into ankitects:main Sep 22, 2024
1 check passed
@dae
Copy link
Member

dae commented Sep 22, 2024

(further changes welcome in follow-up PRs)

@L-M-Sherlock L-M-Sherlock deleted the Feat/true-retention-stats branch September 22, 2024 09:07
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.

4 participants