-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Refactor code #75
base: main
Are you sure you want to change the base?
Refactor code #75
Conversation
Hey Victor, I should be able to look at this PR in about two or three days, since I'm currently travelling at the moment. But as for initial thoughts: we are likely going to be adding an optimizer to this package in the coming month which may change the codebase a fair bit so I'd be a bit hesitant to do another refactoring right away. I'd feel more comfortable to implement the optimizer first, before any refactoring. |
Hi Josh, thanks for your reply. I'll take this couple days to clean up the PR and implement some other improvements I still have in mind. I understand that adding the optimizer is the priority, but if the refactor could cut code size by half and simplify the logic, it may actually be a good idea to do the refactor before adding new code. Your call! I noticed when tweaking the code that tests are not completely exhaustive: some logic changes resulted in tests passing anyway. I wonder: is there some related project (from another programming language) that we could port tests from? If so, I'd be happy to contribute some tests too to make the refactor safer. |
Hi, I've followed this repository for a while and thought I'd contribute some refactors of the code. I have tried to keep the logic the same (haven't changed the tests) but simplified the logic wherever possible and reduced the amount of repeated code.
This is a work in progress, but I'd like to know if work like this would be welcome and take any suggestions.
My plan was to simplify the code as much as possible and then port it to my other project simple-spaced-repetition, which has a more minimalistic API (single
Card
class).Update: this ended up being an almost complete rewrite of the code, but keeping the same functionality. Let me summarize the changes and the logic behind them.
Code split in 3 files
I have split the code in 3 files:
models.py
: which has theRating
,State
,Card
andReviewLog
classes. These classes just serve the purpose of holding the data and its serialization, but there is no business logic.fsrs_v5.py
: a newFSRSv5
class that encapsulates all the FSRS v5 logic. To put it in another way, this is the place where all the 19 weights are used, and only the 19 weights are used.scheduler.py
: theScheduler
class implements the top level class that users should use. It is a wrapper around FSRS v5 that adds learning/relearning steps, fuzzing and interval clamping.Test changes
The tests have stayed the same except for two cases:
fsrs
attribute with aFSRSv5
object. The attributes' comparison when serializing/deserializing failed because objects had a different address.ValueError
when using a naivedatetime
, I believe it is more convenient to just add aUTC
timezone to thedatetime
. Also, if thedatetime
is timezone aware but has a different timezone, we can use it directly.Important refactors
The logic stays the same for the rest of the code, but I would highlight some of the improvements on the new approach:
review_card
. Before it was a huge function, now it is much smaller and easier to understand.update_from_steps
function, that is used both for learning and relearning steps.I know this is a huge rewrite, but hopefully you find it useful. Even if you don't feel like merging all of it, I think it can provide some ideas on how to improve the code.