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

polish graphs of simulator, true retention table and forgetting curve #3448

Merged

Conversation

L-M-Sherlock
Copy link
Contributor

@L-M-Sherlock L-M-Sherlock commented Sep 27, 2024

Source: https://forums.ankiweb.net/t/anki-24-10-beta/49989/23?u=l.m.sherlock

Simulator: “Simulation” is not cut off now.

image

True retention: the width is more adaptive.

image

image

image

Forgetting curve: don't display time, only date when the total elapsed time is >=365 days AND the currently selected timeframe is either “First year”.

image image

also fix:

@Expertium
Copy link
Contributor

Expertium commented Sep 27, 2024

image
Stability and retrievability in the graph don't match the ones at the top of Card Info.
I could maybe write the difference in retrievability off as a rounding error, but stability is definitely wrong.
EDIT: "FSRS: update memory state and reschedule" fixes the problem. Strange. I don't know why my memory states would be inaccurate.

@Expertium
Copy link
Contributor

Expertium commented Sep 27, 2024

Ok, here's an actual problem:
https://imgur.com/hsW6N2o

The last segment of the forgetting curve makes the mouse and the pop-up window with stats glitch
Also, suggestion: decrease precision of percentages in the True Retention table from 2 digits after the decimal point to one. Simply put, from 85.59% to 85.6%

@brishtibheja
Copy link
Contributor

Can we not divide the true retention table into three parts and have radio buttons to switch between them? Thoughts?

@L-M-Sherlock L-M-Sherlock marked this pull request as ready for review September 28, 2024 05:39
@Expertium
Copy link
Contributor

Can we not divide the true retention table into three parts and have radio buttons to switch between them? Thoughts?

Idk, I'm not sure that it's a good idea, but I'm not sure if it's a bad idea either. @user1823 thoughts?

@brishtibheja
Copy link
Contributor

@Expertium It can be done only on narrow devices if possible. IMO it's better than scrolling. But, on wider screens, do you really need to see everything at once or is it better to have limited info on focus? This is the crux we need to consider even for narrow screen.

It's fine if Sherlock doesn't want to do it if it can be done later. I want to hear dae's opinions.

@user1823
Copy link
Contributor

It can be done only on narrow devices if possible. IMO it's better than scrolling.

I agree. If we can't fit whole table on the screen, having radio buttons to switch is better than scrolling.

@voczi
Copy link
Contributor

voczi commented Sep 30, 2024

Two questions for the simulator:

  1. Do simulation days less than 25 work now with this PR?
  2. Can you use something like 777 simulation days without the bottom axis labels overlapping with each other?

For 1) I had a solution of just doing Math.ceil(days, 7) in that one function where the graph is created. Not sure if this is right or not, but anyway, the original Math.round(days/50) will just result in 0 for days<25.
Although for 2) I was looking at the d3 docs for hours without finding a proper solution.

@L-M-Sherlock
Copy link
Contributor Author

  1. Thanks for your solution! It looks nice now.
image
  1. I cannot reproduce the overlapping problem.
image

@voczi
Copy link
Contributor

voczi commented Sep 30, 2024

2. I cannot reproduce the overlapping problem.

The graphs axis looks different now. So maybe it's gone.

@Expertium
Copy link
Contributor

Expertium commented Sep 30, 2024

Btw, it might be worth doing some light theme - dark theme adjustments because "dark on dark" is not good, UI-wise
The text and the curve should be white, or close to white
image

@L-M-Sherlock
Copy link
Contributor Author

image

Done.

@Expertium
Copy link
Contributor

The curve itself is still "dark on dark". How complicated would it be to define, say, 20 light colors for the dark theme and 20 dark colors for the light theme?

@L-M-Sherlock
Copy link
Contributor Author

The choices are limited: https://d3js.org/d3-scale-chromatic/categorical

I'm not a professional UI designer. Suggestion is welcome.

@L-M-Sherlock
Copy link
Contributor Author

image

I figure it out.

@voczi
Copy link
Contributor

voczi commented Sep 30, 2024

Everything looks a lot better now imo.

@dae
Copy link
Member

dae commented Sep 30, 2024

The minimum width of the graph is still too wide unfortunately, so #3445 hasn't been solved yet. An easy way to test this locally is to start Anki with:

ANKI_API_HOST=0.0.0.0 ./run

Then in Chrome, surf to http://10.0.0.2:40000/graphs (or the IP address of your computer). You can then resize the window to see how it looks on a mobile screen. Or you can paste that URL into Safari on an iPhone, and see it displayed there.

Note how the other graphs have scrolled off to the right:

Screenshot 2024-10-01 at 12 06 49 am

Radio buttons were suggested earlier, and they're a reasonable solution if the user will typically only be looking at one of the tree (e.g. mature). If it's common for users to want to see all the data at once, perhaps it would be better to put the three tables in a flexbox, and have them automatically wrap on narrow devices, and display side by side on wider screens?

Thank you for all the hard work you've put into these fixes!

@dae dae merged commit 59969f6 into ankitects:main Sep 30, 2024
1 check passed
@L-M-Sherlock L-M-Sherlock deleted the polish-graphs-of-simulator-and-forgetting-curve branch October 21, 2024 06:52
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.

6 participants