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.

Sign up for GitHub

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

Jump to bottom

add minute_offset and update to fsrs 0.6.2 #25

Merged
merged 1 commit into from
Apr 28, 2024
Merged

add minute_offset and update to fsrs 0.6.2 #25

merged 1 commit into from
Apr 28, 2024

Conversation

AlexErrant
Copy link
Member

Closes #22

Answering some questions from here:

The minute_offset is not very readable.

Agreed.

Could you provide an example or document?

Please let me know if this PR's example/documentation is insufficient.

If the previous next_day_starts_at is 4 and the timezone is UTC+8:00, what's the minute_offset for current method?

I kinda show this in train.ts. But to be explicit:

minute_offset = utc_offset - next_day_starts_at
minute_offset = 8hr - 4hr
minute_offset = 240 minutes

AlexErrant
AlexErrant commented Apr 27, 2024
View reviewed changes
sandbox/src/train.ts
Comment on lines -138 to +156
let parameters = fsrs.computeParametersAnki(cids, eases, ids, types, progress)
// MDN states:
// The number of minutes returned by getTimezoneOffset() is positive if the
// local time zone is behind UTC, and negative if the local time zone is ahead of UTC.
// This is useful when going from our timezone to UTC; our local time + offset = UTC.
// However, we want to go from UTC to our timezone. (Anki revlog id is UTC, and we want to convert it to local time.)
// So we flip the sign of `getTimezoneOffset()`.
let timeZoneMinutesOffset = new Date().getTimezoneOffset() * -1
let ankiNextDayStartsAtMinutes = 4 * 60
let parameters = fsrs.computeParametersAnki(
// We subtract ankiNextDayStartsAtMinutes because
// we want a review done at, say, 1am to be done for the *prior* day.
// Adding would move it into the future.
timeZoneMinutesOffset - ankiNextDayStartsAtMinutes,
cids,
eases,
ids,
types,
progress,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know if the logic in these comments is confusing or wrong!

@AlexErrant AlexErrant mentioned this pull request Apr 27, 2024
Merged
@L-M-Sherlock
Copy link
Member

L-M-Sherlock commented Apr 28, 2024 •
edited
Loading

@sobjornstad is using our package in their product (@remnoteio), so I'd like to ask for his advice.

L-M-Sherlock
L-M-Sherlock approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@L-M-Sherlock L-M-Sherlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ishiko732
ishiko732 approved these changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@ishiko732 ishiko732 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sandbox/src/train.ts
// We subtract ankiNextDayStartsAtMinutes because
// we want a review done at, say, 1am to be done for the *prior* day.
// Adding would move it into the future.
timeZoneMinutesOffset - ankiNextDayStartsAtMinutes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks fine. If my timezone is UTC with a utc_offset = 0 and next_day_starts_at = 4, then the minute_offset = 240 minutes. find the relevant code and context at this link https://github.com/open-spaced-repetition/fsrs-rs/blame/14a62849af95a0df0f97cc1251390d03a1dda19c/src/convertor_tests.rs#L78-L82

So, if I review at 2024-04-28 05:00, the converted time would be 2024-04-28 01:00. And if I review at 2024-04-28 03:00, the converted time would be 2024-04-27 23:00. Is that correct?

Copy link
Member

@L-M-Sherlock L-M-Sherlock Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's intended. Ideally, we hope the user doesn't do their reviews around the NextDayStarts, when they should be sleeping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my timezone is UTC with a utc_offset = 0 and next_day_starts_at = 4, then the minute_offset = 240 minutes.

To be exceedingly pedantic, minute_offset = -240 minutes; i.e. it's negative. This can be seen in the original code where next_day_starts_at is subtracted. In this PR's code it's changed to addition, so the subtraction must be done client-side (in Javascript) as explained in the comments.

The rest is correct.

@L-M-Sherlock L-M-Sherlock merged commit b1958a6 into main Apr 28, 2024
1 check passed
@L-M-Sherlock L-M-Sherlock deleted the add_minute_offset branch April 28, 2024 13:08
@L-M-Sherlock
Copy link
Member

Do you have a plan for the next release?

@AlexErrant
Copy link
Member Author

Yup gimme a sec!

@AlexErrant
Copy link
Member Author

I'm not sure if sobjornstad/Remnote uses the Anki revlog format. This PR is only relevant to those using that style; using raw fsrs-items means you get to control the timestamps/card-grouping however you wish.

(Editorial time: Honestly, using raw fsrs-items is probably the best way to do things. There are some drawbacks to using Anki's format. For example, if you went to Europe for summer vacation, then went home in autumn, each timezone shift would change your training data's date_naive(). This is annoying, but probably doesn't significantly effect training. It just bothers some people that their heatmaps are now missing spots haha.)

@L-M-Sherlock
Copy link
Member

Yeah. Timezone shift is hard to deal with if we use the revlog format. I don't know how Anki deal with that. @dae, sorry for bothering you. Has Anki solved this issue? If so, how?

@dae
Copy link

dae commented May 1, 2024

Anki revlogs do not record timezone info, so Anki is not aware of historical timezone changes.

@L-M-Sherlock
Copy link
Member

Fine. Thanks for that information.

@sobjornstad
Copy link

Sorry for the delay, I didn't check my Discord messages for a while :)

What is the “Anki revlog format” in the context of FSRS? Some choice in the API used? We use FSRS.repeat() to replay RemNote's history format onto an FSRS card object, and we pass in the UTC time that the review was conducted. No time zone is recorded in our review logs either.

@AlexErrant
Copy link
Member Author

Ah, "Anki revlog format" in the context of fsrs-browser means data that's more or less shaped like the revlog table in Anki. To be specific, the computeParametersAnki and memoryStateAnki methods in lib.rs (which are exposed/exported to Javascript) require cids, eases, ids, and types.

I think FSRS.repeat() is a py-fsrs thing? Since fsrs-browser is based on fsrs-rs which doesn't have a repeat, perhaps this discussion is entirely irrelevant to your interests haha.

@sobjornstad
Copy link

We use ts-fsrs, not py-fsrs. The only factors involved in using repeat are the date of the review and the rating provided.

@L-M-Sherlock
Copy link
Member

@sobjornstad, this PR is related to the optimization, so you need to check that part of RemNote rather than scheduler.

@sobjornstad
Copy link

sobjornstad commented May 17, 2024 •
edited
Loading

Ah gotcha, I have actually not been involved in the optimizer at all yet. I'll review that and check with Martin tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@L-M-Sherlock L-M-Sherlock L-M-Sherlock approved these changes

@ishiko732 ishiko732 ishiko732 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] The hard-coded timezone in anki_to_fsrs
5 participants
@AlexErrant @L-M-Sherlock @dae @sobjornstad @ishiko732