Feat/Per card parameters for simulator

[Luc-Mcgrady]

I'm not sure if the parameters and desired retention would be better in an option? That way they can be opted out of for the values passed to the function maybe?

[gemini-code-assist]

Summary of Changes

Hello @Luc-Mcgrady, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a fundamental change to the simulator's parameter handling by enabling each individual card to carry its own set of parameters. This enhancement moves away from a monolithic global parameter approach, paving the way for more nuanced and flexible simulations where different cards can adhere to distinct learning models or weightings. The modifications involve structural changes to the Card type, adjustments to function interfaces to leverage card-specific parameters, and ensuring proper parameter propagation during card creation and processing.

Highlights

• Per-Card Parameters: Each Card struct now includes its own parameters: Arc<Vec<f32>> field, allowing individual cards to maintain distinct sets of model weights for calculations.
• Simplified Function Signatures: Functions and closures like ReviewPriorityFn, retention_on, and retrievability have been updated to access parameters directly from the Card object, removing the need to pass a global Parameters reference explicitly.
• Parameter Initialization and Usage: New cards created during simulation are initialized with a clone of the global parameters, and all memory model calculations now consistently use the parameters stored within each Card.
• New Test Case: A new test, test_per_card_parameters, has been added to validate that cards can indeed have unique parameters and that these parameters correctly influence their simulated behavior, such as interval calculations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces the ability to specify FSRS parameters on a per-card basis, which is a valuable enhancement for simulations. The implementation is mostly on the right track, with updates to data structures and function signatures to accommodate this change. However, I've identified a critical bug in expected_workload_with_existing_cards where the WorkloadEstimator is not correctly used when cards have different parameter sets, leading to incorrect calculations. I've also provided a suggestion to improve the Card API to make it more robust and prevent potential misuse. Addressing these points will significantly improve the correctness and maintainability of the new feature.

[gemini-code-assist]

There is a critical issue here. The WorkloadEstimator is initialized and its cost matrix is precomputed using a single set of parameters w. However, the function then processes cards that may have their own parameters, which can differ from w.

The evaluate_in_flight_card_cost and evaluate_new_card_cost methods rely on the precomputed cost_matrix. This will lead to incorrect workload calculations for any card whose parameters do not match w.

To fix this, you need to run the estimation for each unique set of parameters. You can do this by identifying all unique parameter sets, and for each set, filtering the cards that use it and calculating their combined workload.

Here's a possible implementation to replace lines 570-605:

    let mut estimator = WorkloadEstimator::new(config);
    let mut cards = existing_cards.to_vec();
    let w_global = Arc::new(check_and_fill_parameters(parameters)?);

    if config.learn_limit > 0 {
        let init_ratings = (0..(config.deck_size - cards.len())).map(|i| Card {
            id: -(i as i64),
            difficulty: f32::NEG_INFINITY,
            stability: f32::NEG_INFINITY,
            last_date: f32::NEG_INFINITY,
            due: (i / config.learn_limit) as f32,
            interval: f32::NEG_INFINITY,
            lapses: 0,
            desired_retention,
            parameters: w_global.clone(),
        });

        cards.extend(init_ratings);
    }

    let mut unique_params: Vec<Arc<Vec<f32>>> = vec![];
    for card in &cards {
        if !unique_params.iter().any(|p| Arc::ptr_eq(p, &card.parameters)) {
            unique_params.push(card.parameters.clone());
        }
    }

    let mut total_workload = 0.0;
    for params in unique_params {
        estimator.precompute_cost_matrix(desired_retention, &params);
        let group_workload: f32 = cards.iter().filter(|c| Arc::ptr_eq(&c.parameters, &params)).map(|card| {
            if card.stability > 1e-9 {
                estimator.evaluate_in_flight_card_cost(card, &card.parameters)
            } else {
                estimator.evaluate_new_card_cost(
                    &card.parameters,
                    &config.first_rating_prob,
                    card.due as usize,
                )
            }
        }).sum();
        total_workload += group_workload;
    }

    return Ok(total_workload);

This is a critical bug that affects the correctness of the workload estimation. Also, consider adding a test case for expected_workload_with_existing_cards with cards that have different parameters to cover this scenario.

[Luc-Mcgrady]

I will give up on the workload version.

[Luc-Mcgrady]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant feature: per-card parameters for the simulator. The changes are mostly well-implemented, updating the Card struct and the simulate function to correctly use these new parameters. However, I've identified a critical issue in expected_workload_with_existing_cards where it fails to use the per-card parameters, instead using global ones. Additionally, I've provided a suggestion to improve the Card struct's API to prevent potential misuse regarding parameter validation.

[L-M-Sherlock]

I'm not sure if the parameters and desired retention would be better in an option? That way they can be opted out of for the values passed to the function maybe?

Which structure of function did you mean?

[Luc-Mcgrady]

Wrap these in an Option and use the defaults in the function if it's None.

[L-M-Sherlock]

It depends on how we call this API in Anki's side. How do you think?

[Luc-Mcgrady]

Will it be implemented in Anki before you release it? Then we can change it to an Option if it is easier.

[L-M-Sherlock]

I haven't figured out how to design an UI for the simulator which can include multiple presets. Do you have any idea? Btw, I it's not very urgent for me to make a new release. Burn recently released 0.20, I would take a look when I'm available.

[Luc-Mcgrady]

I think maybe we could just have a search-box in the simulator modal and the cards that it matches regardless of preset?
Also I'm not sure how to use the per deck DR with the simulators DR setting.

[L-M-Sherlock]

how to use the per deck DR with the simulators DR setting.

Can we just store the DR in Card structure?

[Luc-Mcgrady]

I mean if we have per deck DR then how should we handle the DR setting in the simulator?

[L-M-Sherlock]

OMG... I haven't thought about it.

[L-M-Sherlock]

Let's merge it and leave the remaining issues to the next PR.