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

Achieve parity with the Python optimizer #88

Closed
dae opened this issue Sep 28, 2023 · 10 comments · Fixed by #119
Closed

Achieve parity with the Python optimizer #88

dae opened this issue Sep 28, 2023 · 10 comments · Fixed by #119

Comments

@dae
Copy link
Collaborator

dae commented Sep 28, 2023

Currently the generated weights from this crate are slightly behind the Python optimizer. Not urgent, but in the long run, it would be nice if this crate could perform as well.

https://github.com/open-spaced-repetition/fsrs-benchmark#weighted-by-number-of-reviews

Any thoughts on what might be driving the differences?

@L-M-Sherlock
Copy link
Member

L-M-Sherlock commented Sep 28, 2023

@user1823
Copy link
Contributor

The current outlier filter only remove those outliers in pretrain-set. The py version removes the them in the train-set, too

In my opinion, the current outlier filter is too aggressive (it removes too many reviews). This is fine for the pretrain function (because the pretrain method is fragile). However, the train function should have access to more reviews. So, I think that we should build a less aggressive outlier filter for the train function.

There is no hurry in doing this. But, I wanted to make this point here so that you don't waste time and effort in applying the current outlier filter to the trainset.

@user1823
Copy link
Contributor

user1823 commented Oct 2, 2023

The python optimizer selects the parameters from the epoch with the minimum loss. I don't think that the rust optimizer behaves similarly.

Related commit: open-spaced-repetition/fsrs4anki@1719dae

@L-M-Sherlock
Copy link
Member

The python optimizer selects the parameters from the epoch with the minimum loss. I don't think that the rust optimizer behaves similarly.

I also find out the difference, but the framework also doesn't also me to implement a similar feature.

@dae
Copy link
Collaborator Author

dae commented Oct 2, 2023

We could always switch to our own training loop if need be. @nathanielsimard I presume something like this is not supported out of the box at the moment?

@nathanielsimard
Copy link

I'm going to prioritize adding early stoping within burn-train, I think it's time we support that feature!

@nathanielsimard
Copy link

Burn Issue: tracel-ai/burn#841

@user1823
Copy link
Contributor

In case you aren't already aware, I wanted to let you know that Burn now supports early stopping when training, implemented in tracel-ai/burn#878. Probably, now is the time to implement early stopping in the Rust version of the FSRS optimizer.

By the way, the comparison between the Python and the Rust versions of the optimizer is somewhat unfair.

The Python version filters several reviews before (training and) evaluation which are not filtered out by the Rust version. Examples include:

  • Reviews of cards that are filtered by the outlier filter in pre-train.
  • Reviews of cards with incomplete revlogs.

In my opinion, evaluation should happen for these reviews, which means that the behavior of the Rust version is desirable. However, we should keep in mind that it is inconsistent with the Python version.

@dae
Copy link
Collaborator Author

dae commented Oct 22, 2023

tracel-ai/burn#882 will need to be merged before we can update burn

@L-M-Sherlock
Copy link
Member

The Python version filters several reviews before (training and) evaluation which are not filtered out by the Rust version. Examples include:

The python version doesn't filter any reviews before training and evaluation in the benchmark. Here is the code:

https://github.com/open-spaced-repetition/fsrs-benchmark/blob/8aacff613116d70ab21325628235a38024d57d48/script.py#L120-L148

@L-M-Sherlock L-M-Sherlock linked a pull request Nov 20, 2023 that will close this issue
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 a pull request may close this issue.

4 participants