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

[Question] why the seed includes review_time.getTime()? #131

Closed
L-M-Sherlock opened this issue Oct 12, 2024 · 12 comments · Fixed by #137
Closed

[Question] why the seed includes review_time.getTime()? #131

L-M-Sherlock opened this issue Oct 12, 2024 · 12 comments · Fixed by #137
Assignees
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@L-M-Sherlock
Copy link
Member

private initSeed() {
const time = this.review_time.getTime()
const reps = this.current.reps
const mul = this.current.difficulty * this.current.stability
this.algorithm.seed = `${time}_${reps}_${mul}`
}

Does it mean I will get different interval if I review the same card in the same date?

@L-M-Sherlock L-M-Sherlock added the question Further information is requested label Oct 12, 2024
@ishiko732
Copy link
Collaborator

ishiko732 commented Oct 12, 2024

Does it mean I will get different interval if I review the same card in the same date?

Yes, when the interval > 2.5, it is possible to get different intervals on the same day due to variations in seconds. The initial idea was based on the conditions at that moment.

To maintain the same interval within the same day, you can use time % 86400000 (1 day).

@L-M-Sherlock
Copy link
Member Author

The fuzz seed implemented in Anki is generated from card id and the number of reviews of the card. The seed will not change if the user doesn't review it. It's a good design because the fuzz factor will not change over time.

fn get_fuzz_seed_for_id_and_reps(card_id: CardId, card_reps: u32) -> Option<u64> {
    if *crate::PYTHON_UNIT_TESTS || cfg!(test) {
        None
    } else {
        Some((card_id.0 as u64).wrapping_add(card_reps as u64))
    }
}

source: https://github.com/ankitects/anki/blob/f00211df359f8214e9e97688776876a724f59266/rslib/src/scheduler/answering/mod.rs#L602-L609

The current implementation of fuzz seed in ts-fsrs can cause a weird case: the interval may decrease over time. For example, if the original interval is 100 days, and the fuzz factor is 1 in the first day, the fuzzed interval would be ~110 days. If I don't review it and delay the review to the next day, based DSR model, the original interval will increase to ~105 days. But the fuzz factor may decrease to 0, then the fuzzed interval would be ~95 days.

@ishiko732
Copy link
Collaborator

Anki is generated from card id and the number of reviews of the card.

Since the card type in ts-fsrs does not have a card_id, making it incompatible with Anki's approach, should I remove the time variable?

@L-M-Sherlock
Copy link
Member Author

Removing time will solve the aforementioned problem, but cause new problem. this.current.difficulty * this.current.stability is not random if the card's review logs are the same. I haven't figured out any good solution without introducing new property into Card object.

@ishiko732
Copy link
Collaborator

I haven't figured out any good solution without introducing new property into Card object.

How about directly exposing init_seed, allowing developers to implement it themselves? If init_seed is not implemented, it defaults to the current situation.

@ishiko732 ishiko732 added the help wanted Extra attention is needed label Oct 12, 2024
@jcyuan
Copy link

jcyuan commented Nov 7, 2024

personally i think a card has an id is reasonable. why this solution not work for ts-fsrs?

@ishiko732
Copy link
Collaborator

personally i think a card has an id is reasonable. why this solution not work for ts-fsrs?

For related details, please see: open-spaced-repetition/go-fsrs#3 (comment)

@jcyuan
Copy link

jcyuan commented Nov 8, 2024

personally i think a card has an id is reasonable. why this solution not work for ts-fsrs?

For related details, please see: open-spaced-repetition/go-fsrs#3 (comment)

i got it, thank you.
so maybe consider some soltuions like providing base virtual method to override by users.

@ishiko732
Copy link
Collaborator

so maybe consider some soltuions like providing base virtual method to override by users.

Plan to use the proxy design pattern to implement this feature, which allows developers to use Reflect.get to retrieve the card.card_id passed into the repeat/next method and permits overriding the initSeed method.

@jcyuan
Copy link

jcyuan commented Nov 18, 2024

so maybe consider some soltuions like providing base virtual method to override by users.

Plan to use the proxy design pattern to implement this feature, which allows developers to use Reflect.get to retrieve the card.card_id passed into the repeat/next method and permits overriding the initSeed method.

oh, i didn't realize there is Reflect.get method, actually i can hold a reference of the parameters passed to the methods, but with this the way is more elegant, yes.
thanks Ishiko.

@ishiko732 ishiko732 self-assigned this Nov 24, 2024
@ishiko732
Copy link
Collaborator

ishiko732 commented Nov 24, 2024

but with this the way is more elegant, yes.

I plan to write the GenSeedStrategyWithCardId method, which will allow developers to quickly implement logic to read the card_id field and replace the seed handling. This can be easily integrated into projects:

//  Generates a seed strategy function for card IDs.
export function GenSeedStrategyWithCardId(card_id_field: string): TSeedStrategy {
  return function (this: AbstractScheduler): string {
    // https://github.com/open-spaced-repetition/ts-fsrs/issues/131#issuecomment-2408426225
    const card_id = Reflect.get(this.current, card_id_field) ?? 0
    const reps = this.current.reps
    // ex1
    // card_id:string + reps:number = 'e2ecb1f7-8d15-420b-bec4-c7212ad2e5dc' + 4
    // = 'e2ecb1f7-8d15-420b-bec4-c7212ad2e5dc4'

    // ex2
    // card_id:number + reps:number = 1732452519198 + 4
    // = '17324525191984'
    return String(card_id + reps || 0)
  }
}

function main() {
  const seedStrategy = GenSeedStrategyWithCardId('card_id')
  const f = fsrs().useStrategy(StrategyMode.SEED, seedStrategy)
  const card = createEmptyCard<Card & { card_id: number }>()
  card.card_id = 555
  const record = f.repeat(card, new Date())
  for (const item of record) {
    console.log(item)
  }
}

@ishiko732
Copy link
Collaborator

so maybe consider some soltuions like providing base virtual method to override by users.

Plan to use the proxy design pattern to implement this feature, which allows developers to use Reflect.get to retrieve the card.card_id passed into the repeat/next method and permits overriding the initSeed method.

oh, i didn't realize there is Reflect.get method, actually i can hold a reference of the parameters passed to the methods, but with this the way is more elegant, yes. thanks Ishiko.

To quickly migrate to using the card_id field as the seed, you can refer to this commit: ishiko732/example-ts-fsrs-edge-functions@c8eedfa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants