Refactor/extract standalone functions from structure by L-M-Sherlock · Pull Request #367 · open-spaced-repetition/fsrs-rs

L-M-Sherlock · 2025-09-29T03:51:43Z

This PR introduces a significant refactoring of the FSRS codebase, making the API more ergonomic and consistent while extracting standalone functions from the FSRS structure. These changes warrant a major version bump from 5.2.0 to 6.0.0.

Key changes:

API improvements:
- Added FSRS::default() for simpler initialization
- Changed FSRS::new() to take parameters directly instead of Option<&Parameters>
- Simplified FSRS structure by making the model field non-optional
Extracted standalone functions:
- compute_parameters
- evaluate_with_time_series_splits
- optimal_retention
- current_retrievability
Code organization:
- Test functions renamed with test_ prefix
- Internal module structure improved with better visibility controls
- Dependency imports reorganized for clarity

Migration guide:

FSRS Instance Creation

Before:

// With default parameters
let fsrs = FSRS::new(Some(&[])).unwrap();
// or
let fsrs = FSRS::new(None).unwrap();

// With custom parameters
let fsrs = FSRS::new(Some(&parameters)).unwrap();

After:

// With default parameters
let fsrs = FSRS::default();

// With custom parameters
let fsrs = FSRS::new(&parameters).unwrap();

Optimizing Parameters

Before:

let fsrs = FSRS::new(None).unwrap();
let parameters = fsrs.compute_parameters(ComputeParametersInput {
    train_set: items,
    ..Default::default()
}).unwrap();

After:

let parameters = compute_parameters(ComputeParametersInput {
    train_set: items,
    ..Default::default()
}).unwrap();

Evaluating with Time Series Splits

Before:

let fsrs = FSRS::new(None).unwrap();
let evaluation = fsrs.evaluate_with_time_series_splits(input, |progress| true).unwrap();

After:

let evaluation = evaluate_with_time_series_splits(input, |progress| true).unwrap();

Calculating Optimal Retention

Before:

let fsrs = FSRS::new(None).unwrap();
let optimal = fsrs.optimal_retention(&config, &[], |_| true, None, None).unwrap();

After:

let optimal = optimal_retention(&config, &[], |_| true, None, None).unwrap();

Retrievability Calculation

Before:

let fsrs = FSRS::new(None).unwrap();
// Days
let r = fsrs.current_retrievability(state, days_elapsed, decay);
// Seconds
let r = fsrs.current_retrievability_seconds(state, seconds_elapsed, decay);

After:

// Days
let r = current_retrievability(state, days_elapsed as f32, decay);
// Seconds (converted to days)
let r = current_retrievability(state, seconds_elapsed as f32 / 86400.0, decay);

…zation, and training modules to `pub(crate)` for better encapsulation.

…'test_' to enhance clarity and maintainability.

L-M-Sherlock · 2025-09-29T04:01:10Z

/gemini review

gemini-code-assist

Code Review

This is a solid refactoring pull request that significantly improves the modularity and overall quality of the codebase. Extracting current_retrievability into a standalone function is a great move for better API design. The accompanying changes, such as adjusting visibility for better encapsulation, simplifying function signatures, and renaming tests for consistency, are all well-executed and contribute to a cleaner, more maintainable codebase. The major version bump is appropriate for the breaking API changes. I have one minor suggestion to remove a redundant import.

src/inference.rs

…up code.

Luc-Mcgrady · 2025-09-29T10:45:52Z

Could you wait for me to implement the per-card DR + (maybe) per-card parameters(?) for the simulator before you publish the next major version?

I might wait for this to merge first so that there are no potential merge conflicts .

L-M-Sherlock · 2025-09-29T15:22:45Z

I will merge this PR and wait for your PR. The next release will not come soon.

Luc-Mcgrady

Should evaluate_with_time_series_splits and optimal_retention be made standalone?

also should we remove the Option in

fsrs-rs/src/model.rs

Line 256 in 1e27198

pub fn new(parameters: Option<&Parameters>) -> Result<Self> {

if the intention is that there are no functions that don't require parameters?

…und parameters in multiple files, enhancing code clarity and consistency.

…ult()` instead of `FSRS::new(&[])`, improving code consistency and readability.

…fault()`, ensuring consistency and improving code readability.

…roving consistency and reducing redundancy in multiple test files.

L-M-Sherlock · 2025-09-30T04:25:11Z

/gemini review

gemini-code-assist

Code Review

This pull request is a significant and well-executed refactoring that improves the modularity and API design of the fsrs crate. By extracting standalone functions from the FSRS struct, the API becomes cleaner and more intuitive. The changes are consistently applied across the codebase, including examples, benchmarks, and tests. The renaming of test functions and adjustment of visibility for utility functions are also welcome improvements for code quality. I have one comment regarding device handling which could lead to a runtime panic in some scenarios.

gemini-code-assist · 2025-09-30T04:27:10Z

src/model.rs

-            None => None,
-        };
+        let parameters = check_and_fill_parameters(parameters)?;
+        let model = parameters_to_model::<B2>(&parameters);


The parameters_to_model function creates tensors on the default device, but new_with_backend can be called with a non-default device. This will lead to a panic if the devices don't match when performing operations.

The device should be passed to parameters_to_model and used when creating tensors to ensure they are on the correct device.

You'll need to update parameters_to_model to accept a device parameter. This will likely require a few knock-on changes, for example:

// in src/model.rs pub(crate) fn parameters_to_model<B: Backend>(parameters: &Parameters, device: &B::Device) -> Model<B> { let config = ModelConfig::default(); // Model::new also needs to be updated to accept a device let mut model = Model::new(config.clone(), device); model.w = Param::from_tensor(Tensor::from_floats( TensorData::new( clip_parameters(parameters, config.num_relearning_steps, Default::default()), Shape { dims: vec![21] }, ), device, )); model }

Suggested change

let model = parameters_to_model::<B2>(&parameters);

let model = parameters_to_model::<B2>(&parameters, &device);

@AlexErrant, does it affect fsrs-browser?

asukaminato0721 · 2025-09-30T05:39:33Z

src/training.rs


                // evaluate
-                let model = FSRS::new(Some(&parameters)).unwrap();
+                let model = FSRS::new(&parameters).unwrap();


Good change here.

…nsor creation, enhancing device management and consistency across the codebase.

L-M-Sherlock · 2025-09-30T08:05:20Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant and well-executed refactoring of the FSRS codebase. The key changes, such as extracting standalone functions and simplifying the FSRS struct and its API, make the library more ergonomic and improve its overall structure. The move to a non-optional model field in FSRS and the introduction of FSRS::default() are excellent improvements. The code changes are consistent with the goals outlined in the description. My review includes a few minor suggestions to improve code consistency and clean up test code.

src/inference.rs

gemini-code-assist · 2025-09-30T08:07:30Z

src/simulation.rs

-            .unwrap();
-        dbg!(optimal_retention);
+        let retention_value = optimal_retention(&config, &[], |_| true, None, None).unwrap();
+        dbg!(retention_value);


This dbg! macro appears to be for debugging purposes. It should be removed from the test code to keep the test output clean, especially since there's already an assertion for retention_value on line 2036.

gemini-code-assist · 2025-09-30T08:07:30Z

src/training.rs

+                    num_relearning_steps: None,
+                })
+                .unwrap();
                dbg!(&parameters);


This dbg! macro seems to be for debugging. It should be removed from the test code to keep the output clean. If specific values need to be checked, assertions should be used instead.

gemini-code-assist · 2025-09-30T08:07:30Z

src/training.rs

-                let model = FSRS::new(Some(&parameters)).unwrap();
+                let model = FSRS::new(&parameters).unwrap();
                let metrics = model.evaluate(items.clone(), |_| true).unwrap();
                dbg!(&metrics);


This dbg! macro seems to be for debugging. It should be removed from the test code to keep the output clean. If specific values need to be checked, assertions should be used instead.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…cing code consistency across multiple files.

…e parameter, improving code consistency and simplifying tensor creation across multiple files.

L-M-Sherlock · 2025-09-30T09:19:32Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant and well-executed refactoring of the FSRS codebase. The primary changes involve extracting several key functions (compute_parameters, evaluate_with_time_series_splits, optimal_retention, current_retrievability) into standalone functions, which improves modularity and API ergonomics. The introduction of FSRS::default() and the simplification of FSRS::new are welcome improvements. The code organization has also been enhanced by reducing visibility of internal modules and standardizing test function names.

My review has identified one high-severity issue related to device handling. The refactoring removed the device field from the FSRS struct, but the new device() method incorrectly returns the default device, which could lead to panics if a non-default device is used. I have provided a specific suggestion to fix this.

src/model.rs

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…iple files for improved consistency and clarity.

L-M-Sherlock · 2025-09-30T09:30:09Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant and well-executed refactoring, extracting standalone functions like compute_parameters, evaluate_with_time_series_splits, and optimal_retention from the FSRS struct. The API is now more ergonomic, with the introduction of FSRS::default() and a simplified FSRS::new(). The internal structure of FSRS is also improved by making the model field non-optional and removing the redundant device field. The code organization is enhanced through consistent test naming and better visibility control for internal modules. Overall, these changes are a great improvement to the codebase, making it cleaner, safer, and easier to use for the common cases, justifying the major version bump.

L-M-Sherlock · 2025-09-30T09:36:31Z

@Luc-Mcgrady, the PR is merged now.

L-M-Sherlock added 4 commits September 29, 2025 11:01

Refactor/extract standalone functions from structure

d14efc2

Change visibility of several functions in dataset, parameter initiali…

0a6ce4d

…zation, and training modules to `pub(crate)` for better encapsulation.

Rename test functions for consistency across modules, prefixing with …

3471051

…'test_' to enhance clarity and maintainability.

bump version

782b85f

gemini-code-assist bot reviewed Sep 29, 2025

View reviewed changes

src/inference.rs Outdated Show resolved Hide resolved

Remove unused import of izip in measure_a_by_b function to clean …

5d9cdf4

…up code.

L-M-Sherlock requested a review from asukaminato0721 September 29, 2025 15:27

Luc-Mcgrady reviewed Sep 29, 2025

View reviewed changes

L-M-Sherlock and others added 10 commits September 30, 2025 09:46

make optimal_retention standalone

58ed43d

make evaluate_with_time_series_splits and compute_parameters standalone

c956bd8

[autofix.ci] apply automated fixes

cc76fa6

Refactor FSRS initialization to remove unnecessary Some wrapper aro…

95133c1

…und parameters in multiple files, enhancing code clarity and consistency.

Refactor FSRS initialization across multiple files to use `FSRS::defa…

5408405

…ult()` instead of `FSRS::new(&[])`, improving code consistency and readability.

make benchmark standalone & remove redundant documents

84c16d3

Refactor FSRS initialization in benchmarks and tests to use `FSRS::de…

bec3e40

…fault()`, ensuring consistency and improving code readability.

[autofix.ci] apply automated fixes

5375de0

Refactor tests to use a static DEVICE for NdArrayDevice::Cpu, imp…

f719e86

…roving consistency and reducing redundancy in multiple test files.

[autofix.ci] apply automated fixes

491dd04

gemini-code-assist bot reviewed Sep 30, 2025

View reviewed changes

asukaminato0721 reviewed Sep 30, 2025

View reviewed changes

asukaminato0721 approved these changes Sep 30, 2025

View reviewed changes

Refactor FSRS and Model implementations to utilize device-specific te…

3c9328b

…nsor creation, enhancing device management and consistency across the codebase.

gemini-code-assist bot reviewed Sep 30, 2025

View reviewed changes

L-M-Sherlock and others added 2 commits September 30, 2025 16:28

Update src/inference.rs

bce1023

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Refactor FSRSBatcher initialization to remove device parameter, enhan…

16d4de5

…cing code consistency across multiple files.

Refactor BatchTensorDataset and MemoryStateTensors to eliminate devic…

bb46d34

…e parameter, improving code consistency and simplifying tensor creation across multiple files.

gemini-code-assist bot reviewed Sep 30, 2025

View reviewed changes

src/model.rs Outdated Show resolved Hide resolved

L-M-Sherlock and others added 2 commits September 30, 2025 17:25

Update src/model.rs

3416245

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Refactor device parameter handling in FSRS initialization across mult…

8374800

…iple files for improved consistency and clarity.

gemini-code-assist bot reviewed Sep 30, 2025

View reviewed changes

L-M-Sherlock merged commit 163f031 into main Sep 30, 2025
4 checks passed

L-M-Sherlock deleted the Refactor/extract-standalone-functions-from-structure branch September 30, 2025 09:36

	let model = parameters_to_model::<B2>(&parameters);
	let model = parameters_to_model::<B2>(&parameters, &device);

Uh oh!

Conversation

L-M-Sherlock commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key changes:

Migration guide:

FSRS Instance Creation

Before:

After:

Optimizing Parameters

Before:

After:

Evaluating with Time Series Splits

Before:

After:

Calculating Optimal Retention

Before:

After:

Retrievability Calculation

Before:

After:

Uh oh!

L-M-Sherlock commented Sep 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Luc-Mcgrady commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

L-M-Sherlock commented Sep 29, 2025

Uh oh!

Luc-Mcgrady left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

L-M-Sherlock commented Sep 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

L-M-Sherlock Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

asukaminato0721 Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

L-M-Sherlock commented Sep 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

L-M-Sherlock commented Sep 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

L-M-Sherlock commented Sep 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

L-M-Sherlock commented Sep 29, 2025 •

edited

Loading

Luc-Mcgrady commented Sep 29, 2025 •

edited

Loading

Luc-Mcgrady left a comment •

edited

Loading