-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Set mean reversion target to D0(4) #124
Conversation
Supersedes and closes open-spaced-repetition#123
By the way, shouldn't the simulator import the definitions from fsrs-optimizer/src/fsrs_optimizer/fsrs_simulator.py Lines 80 to 132 in 86eeb69
|
Due to type inference problem, it's hard to reuse this function between optimizer and simulator. By the way, you forget to call the member function as self.init_d. |
Thanks. If you don't mind, can you briefly explain what is the difference between self.init_d and init_d? |
Then, should I make the corrections in the simulator too? |
The init_d function is defined under the class FSRS, so it's the member method. It's a concept from object-oriented programming.
Sure, please. I will check it later. |
I have edited the simulator too. But, I don't really understand what is fsrs-optimizer/src/fsrs_optimizer/fsrs_simulator.py Lines 119 to 123 in 40376ed
Also, there is one inconsistency. In the optimizer, mean reversion is using the unclamped value of D0(4). But, in the simulator, mean reversion is using the clamped value of D0(4). Which one do you think is more appropriate? |
I make them consistent in the last commit. |
Rating offset is basically "average grade of same-day reviews, minus 3". |
OK, LGTM. Note that this PR doesn't include updates to the default parameters from #123. |
I just realized that this is not a bug fix and you deliberately used D0(1) for mean reversion previously (Found out by reading the Wiki page). So, have you tried using D0(3) to see if it gives better results? |
We recently tried D0(1) vs D0(3) vs D0(4), and they all give almost the same results. RMSE differs in the 4 digit after the decimal point. So LMSherlock decided to go with D0(4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
66d4695
* Set mean reversion target to D0(4) open-spaced-repetition/fsrs-optimizer#124 * test/update test * update default parameters
Supersedes and closes #123
Coding it like this will allow tweaking the formula further without needing to change the code at two places.