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/only let FSRS take over short-term schedule when steps are empty #3496

Conversation

L-M-Sherlock
Copy link
Contributor

@L-M-Sherlock L-M-Sherlock commented Oct 14, 2024

source: https://forums.ankiweb.net/t/fsrs-5-1d-scheduling-and-learning-steps/50242/72?u=l.m.sherlock

In previous implementation #3375, FSRS will take over the scheduling when the (re)learning steps run out, which is unintended.

However, someone wishes for the previous one: https://forums.ankiweb.net/t/fsrs-5-1d-scheduling-and-learning-steps/50242/77?u=l.m.sherlock

Should we add an extra option to support this case?

@user1823
Copy link
Contributor

Another request:
If w[17] OR w[18] is zero, disable the FSRS short-term scheduler because FSRS won't be able to increase the stability with short-term reviews.

Inspired by https://forums.ankiweb.net/t/fsrs-5-1d-scheduling-and-learning-steps/50242/73

@Expertium
Copy link
Contributor

Should we add an extra option to support this case?

Maybe make a simple command that can be added in the custom scheduling field?

If w[17] OR w[18] is zero, disable the FSRS short-term scheduler because FSRS won't be able to increase the stability with short-term reviews.

Makes sense.

@DerIshmaelite
Copy link

DerIshmaelite commented Oct 14, 2024

Maybe make a simple command that can be added in the custom scheduling field?

But this would not apply to all presets, if you were to have different presets would it :(

Maybe a console command that applies this to all presets...Or an option to apply to all presets...

@DerIshmaelite
Copy link

If w[17] OR w[18] is zero, disable the FSRS short-term scheduler because FSRS won't be able to increase the stability with short-term reviews.

Makes sense.

What about the cases, where the parameters are still 0 even after optimizing, or the cases where no 0 is even added at all (remains at 17 params instead of 19).

image

@Expertium
Copy link
Contributor

or the cases where no 0 is even added at all (remains at 17 params instead of 19).

They are added "behind the scenes", without the user seeing it, which I didn't know at the time when you took this screenshot

@DerIshmaelite
Copy link

DerIshmaelite commented Oct 14, 2024

or the cases where no 0 is even added at all (remains at 17 params instead of 19).

They are added "behind the scenes", without the user seeing it, which I didn't know at the time when you took this screenshot

But why is having the scheduler turned on and having these 0 values exactly bad? Some decks have these 0 values and I wouldn't want the scheduler disabled automatically for them.

@Expertium
Copy link
Contributor

If those parameters are zero, FSRS won't do anything useful with your same-day reviews data

@user1823
Copy link
Contributor

user1823 commented Oct 14, 2024

Because if w[17] or w[18] is 0, the interval will not increase in same-day reviews unless the next review (with the same interval) crosses the "next day starts at" time.

@DerIshmaelite
Copy link

Because if w[17] or w[18] is 0, the interval will not increase in same-day reviews unless the next review (with the same interval) crosses the "next day starts at" time.

Ahhh so it stays frozen. This makes sense.

@mlidbom
Copy link

mlidbom commented Oct 15, 2024

I just want to chime in and say that I would also very much like for the current behavior to remain available somehow. I've only been using this for a short time but I already know I would just hate to see it go away.

Here's why: For some decks I know from experience that I need a quite small, 10m, first (re)learning step, much smaller than the steps FSRS gives me, but after that it's just fabulous to have FSRS add more learning steps when it can tell that I need them. So I'd love for this behavior to remain available somehow.

I tried removing my steps, and my recall for these decks with just FSRS short term scheduling became much worse. It's not so much that I can't recall at all with the FSRS steps, as that I will fail a card that I cannot recall fast, and to get there I need the first small step it seems.

@user1823
Copy link
Contributor

I just want to chime in and say that I would also very much like for the current behavior to remain available somehow.

This request is tracked in

@dae
Copy link
Member

dae commented Oct 15, 2024

This is still an experimental feature, so if you agree this should be done, a debug console command to toggle this behavior seems reasonable. It would be awkward to implement in custom scheduling, as it's not a property of card state.

@L-M-Sherlock would you like me to merge this as-is, or wait for #3497?

@L-M-Sherlock
Copy link
Contributor Author

I suggest merging it firstly and then I will implement the feature in another PR.

@dae dae merged commit f804abf into ankitects:main Oct 15, 2024
1 check passed
@mlidbom
Copy link

mlidbom commented Oct 15, 2024

@L-M-Sherlock
One small suggestion. If it is not much harder, a UI option in the addon would be a great addition to my mind. This keeps the complexity out of Anki proper, while still adding discoverability and simplicity for users of the addon.

mlidbom added a commit to mlidbom/anki that referenced this pull request Oct 16, 2024
…e empty (ankitects#3496)"

This reverts commit f804abf.

I want to keep this functionality :)
The change in the .clamp() value seems unrelated so  I'm keeping that. I'm also keeping the new is_empty() function. No reason to have a bigger diff than necessary.
@L-M-Sherlock L-M-Sherlock deleted the Fix/only-let-FSRS-take-over-short-term-schedule-when-steps-are-empty branch October 16, 2024 01:13
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