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

Feat/add next_state method #146

Merged
merged 14 commits into from
Jan 6, 2025
Merged

Feat/add next_state method #146

merged 14 commits into from
Jan 6, 2025

Conversation

ishiko732
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cdd9158) to head (7c88a70).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #146   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          693       721   +28     
  Branches        76        85    +9     
=========================================
+ Hits           693       721   +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/fsrs/algorithm.ts Show resolved Hide resolved
@ishiko732 ishiko732 marked this pull request as ready for review January 6, 2025 11:40
@ishiko732 ishiko732 requested a review from L-M-Sherlock January 6, 2025 11:40
@ishiko732 ishiko732 changed the title Feat/add next_state method Feat/add next_state method Jan 6, 2025
Copy link
Member

@Luc-Mcgrady Luc-Mcgrady left a comment

Choose a reason for hiding this comment

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

When both stability and difficulty are 0 it returns NaN

test('next_state not NaN', () => {
  
  const f = fsrs()
  const next_state = f.next_state({ stability: 0, difficulty: 0 }, 1, 1)

  console.log(next_state)

  expect(Number.isNaN(next_state.stability)).toBe(false)
})

@ishiko732
Copy link
Collaborator Author

ishiko732 commented Jan 6, 2025

When both stability and difficulty are 0 it returns NaN

When stability=0 or difficulty=0, should it be treated as memory_state=null?

@Luc-Mcgrady
Copy link
Member

If both are 0 I'd say yes, but if just stability or difficulty is 0 and the other isn't its probably unintentional and should throw "invalid memory state" or something?

@ishiko732
Copy link
Collaborator Author

If both are 0 I'd say yes, but if just stability or difficulty is 0 and the other isn't its probably unintentional and should throw "invalid memory state" or something?

ok, I've updated the boundaries to ensure exceptions are thrown.

@ishiko732 ishiko732 requested a review from Luc-Mcgrady January 6, 2025 13:47
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

@Luc-Mcgrady
Copy link
Member

Luc-Mcgrady commented Jan 6, 2025

s Still isn't properly clamped I think. (note newstate to the right)
image
I'll make a unit test really quick

Edit:

  it('clamped s', ()=>{
    const f = fsrs()
    const state = {difficulty: 9.98210112, stability: 0.01020119}
  
    const newState = f.next_state(state, 1, 1)

    expect(newState.stability).toBeGreaterThanOrEqual(0.01);
  })

src/fsrs/algorithm.ts Outdated Show resolved Hide resolved
src/fsrs/algorithm.ts Outdated Show resolved Hide resolved
Copy link
Member

@Luc-Mcgrady Luc-Mcgrady left a comment

Choose a reason for hiding this comment

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

Here's some more descriptive error messages if you want them.
All seems good now 👍

src/fsrs/algorithm.ts Outdated Show resolved Hide resolved
src/fsrs/algorithm.ts Outdated Show resolved Hide resolved
src/fsrs/algorithm.ts Outdated Show resolved Hide resolved
__tests__/algorithm.test.ts Outdated Show resolved Hide resolved
__tests__/algorithm.test.ts Outdated Show resolved Hide resolved
__tests__/algorithm.test.ts Outdated Show resolved Hide resolved
__tests__/algorithm.test.ts Outdated Show resolved Hide resolved
__tests__/algorithm.test.ts Outdated Show resolved Hide resolved
@ishiko732
Copy link
Collaborator Author

Here's some more descriptive error messages if you want them. All seems good now 👍

Thank you! For the cases where other methods (such as generatorParameters) are not correctly clamped, I'll handle them in a subsequent PR.

@ishiko732 ishiko732 merged commit 8355606 into main Jan 6, 2025
3 checks passed
@ishiko732 ishiko732 deleted the Feat/next_state branch January 6, 2025 16:14
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 this pull request may close these issues.

[Question] How should the ds be calculated for cards that have been manually rescheduled?
3 participants