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

Fix/keep two decimals for stability #2873

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

L-M-Sherlock
Copy link
Contributor

Reason: some users reported that the rescheduling results are inconsistent between built-in FSRS and the helper add-on. For example, if the memory stability is 8.48, the built-in FSRS will schedule it with 8 days interval. However, it is stored as 8.5 in database. The helper will round it to 9 days. It would also induce the inconsistency during the fuzzing.

@L-M-Sherlock L-M-Sherlock changed the title Fix/keep two decimals Fix/keep two decimals for stability Dec 5, 2023
@dae dae merged commit 96ae3dc into ankitects:main Dec 6, 2023
@L-M-Sherlock
Copy link
Contributor Author

@dae, I find that the round_to_places couldn't solve the problem completely. For example, if the stability is 20.49875, round_to_places(s, 2) is 20.5, which still causes the inconsistency.

@dae
Copy link
Member

dae commented Dec 14, 2023

Can't you adjust the helper to use the same precision/rounding as Anki instead?

@L-M-Sherlock
Copy link
Contributor Author

Can't you adjust the helper to use the same precision/rounding as Anki instead?

Helper reads the value from database. But the value has been rounded before it is stored in database.

@L-M-Sherlock
Copy link
Contributor Author

For example, if the stability is 20.49875 calculated by built-in FSRS, it will be stored as 20.5. Then, the stability is incorrect in the card info because 20.5 is rounded to 21.

image

@dae
Copy link
Member

dae commented Dec 14, 2023

Ok, I see. We could potentially solve this by rounding somewhere in fsrs-rs, but since we'll hopefully implement the helper's functions natively in Rust in the future, perhaps it would be more efficient to document this limitation, and wait until then?

@L-M-Sherlock
Copy link
Contributor Author

I'm OK. It doesn't affect too many cards.

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.

2 participants