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/forgetting curve in card info #3437

Merged
merged 18 commits into from
Sep 27, 2024

Conversation

L-M-Sherlock
Copy link
Contributor

@Expertium
Copy link
Contributor

Suggestion: add radio buttons "First week", "First month", "First year" and "All time". First week = forgetting curve is plotted based on reviews in the first 7 days. First month = the first 30 days, etc. You get the point.

@L-M-Sherlock
Copy link
Contributor Author

"First year" only shows when the elapsed days exceed 365.

image image

@brishtibheja
Copy link
Contributor

brishtibheja commented Sep 24, 2024

I don't get how is the visualisation helpful at all. This seems better suited for an add-on feature if doable. If there's demand, then a native implementation would make more sense.

@L-M-Sherlock
Copy link
Contributor Author

It will take more works to implement it via add-on because I have to modify some low-level logic of Anki. And it is also UI-intense, so I don't want to re-implement the UI in the add-on again. If this PR is rejected, I won't implement it in the add-on.

@L-M-Sherlock
Copy link
Contributor Author

@dae, should we hide the forgetting curve when the user doesn't enable FSRS? I find that the current implementation still display the forgetting curve and the stability column even for a new collection.

@dae
Copy link
Member

dae commented Sep 25, 2024

Like some of our other graphs, I'm not sure how actionable the data is, but it's certainly interesting. Have you seen much demand for this in the past @L-M-Sherlock? Anyone else want to chime in on whether this should be included? I fear if we add new features each time @Expertium has an idea, we'll quickly run out of room ^_^; But this shouldn't have a large performance or maintenance impact.

Thoughts:

  • Are the extra columns in the table adding much value? They make the screen more information dense, and will be too crowded on mobile devices.
  • When the user clicks on a card with no review history, an empty graph is briefly shown, before it fades to 'no data'. It would be nice if the screen either immediately updated to 'no data', or avoided visual updates before fading to 'no data'.
  • Disabling it when SM2 is enabled is probably a good idea
  • 'Days since first review' is overlapping the axis labels
  • We'll need to make it translatable as a final step

@Expertium
Copy link
Contributor

Expertium commented Sep 25, 2024

should we hide the forgetting curve when the user doesn't enable FSRS?

I'm not Dae, but my answer is "of course". How would it even be plotted without FSRS? It would be meaningless and confusing.

Also, to Dae, regarding demand:
image
image
image

This isn't a lot, but so far it seems like people like it.

@L-M-Sherlock
Copy link
Contributor Author

Enable FSRS:

image

Disable FSRS:

image

@L-M-Sherlock
Copy link
Contributor Author

I polished the feature a little. It looks nicer now.

image image

@brishtibheja
Copy link
Contributor

This isn't a lot, but so far it seems like people like it.

@Expertium as with all things "cool", the initial sanguinity soon disappears and it becomes just another piece graph in Anki. I'm not too against the forgetting curve, this is just an FYI.

Another FYI, we are calling this forgetting curve but it isn't really curved here.

@L-M-Sherlock
Copy link
Contributor Author

L-M-Sherlock commented Sep 27, 2024

Another FYI, we are calling this forgetting curve but it isn't really curved here.

Because the stability is too large and the elapsed time is too short. This chart is significantly curved:

image

@dae
Copy link
Member

dae commented Sep 27, 2024

All the changes look good. The only thing stopping me from merging this in is this one:

Are the extra columns in the table adding much value? They make the screen more information dense, and will be too crowded on mobile devices.

Is there a good argument for keeping them? If not, let's remove them. If so, let's hide them on narrow devices.

@dae
Copy link
Member

dae commented Sep 27, 2024

(having thought about it for a bit, I think the graph provides value even if it's not "actionable" - it helps to better visualize the span between reps, and I suspect it might help some users better understand memory decay/how FSRS schedules things)

@L-M-Sherlock
Copy link
Contributor Author

L-M-Sherlock commented Sep 27, 2024

Are the extra columns in the table adding much value?

The column Elapsed Time could save the time to calculate the real interval between two reviews. It makes the comparison between scheduled interval and real interval more convenient.

The column Stability could help the user to check how mature the card varies during reviews. It's more precise than the Interval because it isn't affected by Desired Retention.

@dae
Copy link
Member

dae commented Sep 27, 2024

I'm going to hide these for now, as I think we need a bit more time to discuss them, and I'd like to build a beta today or tomorrow. For a follow-up commit, some thoughts:

  • "Time" and "Elapsed Time" are next to each other, which is a bit confusing
  • "Interval", "Elapsed Time" and "Stability" are easily confused.
  • "Scheduled" (interval) and "Actual" (elapsed time) is one possible suggestion, but there may be better ones

@dae dae merged commit 7959273 into ankitects:main Sep 27, 2024
1 check was pending
@L-M-Sherlock L-M-Sherlock deleted the Feat/forgetting-curve-in-card-info branch September 27, 2024 09:42
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