search

open-spaced-repetition / py-fsrs

Python Package for FSRS
https://pypi.org/project/fsrs/
MIT License
145 stars 23 forks source link

FSRS-5 #52

Closed joshdavham closed 3 months ago

joshdavham commented 3 months ago

@L-M-Sherlock Hey Jarrett, I was wondering if I could get some help with this.

I spent some time looking at the Rust and Typescript implementations of FSRS-5 and am feeling a bit confused about how to implement the new short-term stability function.

Also, is the new argument passed to the mean_reversion function correct? I couldn't find any note about changes on the FSRS-5 page, but did find these two PRs that made changes: ts-fsrs, fsrs-rs.

L-M-Sherlock commented 3 months ago

Sorry, I forget to update the wiki page. Now it shows the latest formula about mean reversion.

feeling a bit confused about how to implement the new short-term stability function

The new short-term stability function is executed in learning stage:

https://github.com/open-spaced-repetition/ts-fsrs/blob/eee4f0c84f91d36f06f8fd55830f319d6a98e146/src/fsrs/impl/basic_schduler.ts#L69

joshdavham commented 3 months ago

@L-M-Sherlock Before I continue with this PR, could I get you to take a quick look at whether the new mean reversion and short term stability formulas are applied correctly?

ishiko732 commented 3 months ago

I think you can refer to https://github.com/open-spaced-repetition/ts-fsrs/pull/95/files and https://github.com/open-spaced-repetition/ts-fsrs/pull/103/files to implement FSRS-v5.

could I get you to take a quick look at whether the new mean reversion and short term stability formulas are applied correctly?

I have reviewed the new mean reversion and short-term stability formulas, and they be correct.

joshdavham commented 3 months ago

@ishiko732

I'm currently testing this implementation using the same weights and ratings that you were using here and I'm getting different results.

While you were getting these resulting intervals:

[0, 4, 17, 62, 198, 563, 0, 0, 9, 27, 74, 190, 457]

I'm currently getting:

[0, 4, 16, 58, 183, 516, 0, 0, 17, 50, 144, 380, 928]

I went and compared the pre-FSRS-5 implementations of both py-fsrs and ts-fsrs (py-fsrs 2.5.1 and ts-fsrs 3.5.7) and verified that they produce identical intervals so this means that there's currently either something wrong with this fsrs-5 implementation or your ts-fsrs implementation... and I'm not sure which!

Furthermore, I tested these weights and ratings again, but with w17=0 and w18=0 to 'disable' the new short term stability function and still got different results:

ts-fsrs:

[0, 3, 13, 50, 163, 473, 0, 0, 12, 34, 91, 229, 541]

py-fsrs:

[0, 3, 13, 48, 155, 445, 0, 0, 12, 39, 116, 313, 780]

So this likely means that the cause of the problem isn't the short term stability problem alone.

Finally, I played around with ts-fsrs's short term and long term schedulers and I didn't end up making any more progress finding the issue :(

Any idea what might be causing the discrepency between our implementations?

joshdavham commented 3 months ago

Alrighty I think this PR is ready for review!

To summarize this PR:

I'd like some feedback on whether a major version bump here seems appropriate. This PR should only introduce breaking changes for those who were using custom weights, but this is likely very few people in reality. But on the other hand, these are still breaking changes and also it looks like both ts-fsrs and fsrs-rs did major version bumps when introducing fsrs-5.

I'd also like to add a release note for those who may have previously been using custom weights. What do you suggest I write? I was thinking of suggesting setting w17 and w18=0 if they haven't yet computed their new fsrs-5 parameters. I could also point them to the fsrs-optimizer or suggest they revert back to the default parameters for now?

ishiko732 commented 3 months ago

I was thinking of suggesting setting w17 and w18=0 if they haven't yet computed their new fsrs-5 parameters.

In this case, both ts-fsrs and fsrs-rs default to adding two [0.0,0.0].

joshdavham commented 3 months ago

That's a cool idea, but I'm not sure if I want to autofill the last two parameters with 0's if the user specifies 17 parameters instead of 19. I think this could cause some unintended behavior for the user, so I'd personally prefer not to implement that in py-fsrs.

ishiko732 commented 3 months ago

@L-M-Sherlock cc

L-M-Sherlock commented 3 months ago

whether a major version bump here seems appropriate.

It's appropriate.

I'd also like to add a release note for those who may have previously been using custom weights. What do you suggest I write?

We need to inform them that only v5 optimizer could generate correct parameters for them.

joshdavham commented 3 months ago

We need to inform them that only v5 optimizer could generate correct parameters for them.

For sure. I could mention to use the latest version of either fsrs-optimizer or fsrs-rs for the optimizer

ishiko732 commented 3 months ago

I could mention to use the latest version of either fsrs-optimizer or fsrs-rs for the optimizer

By the way: fsrs-browser can also be optimized.😊