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

fix incorrect nested review counts in v2 scheduler #2

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pylib/tests/test_schedv2.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,21 +446,21 @@ def test_review_limits():
tree = col.sched.deck_due_tree().children
# (('parent', 1514457677462, 5, 0, 0, (('child', 1514457677463, 5, 0, 0, ()),)))
assert tree[0].review_count == 5 # parent
assert tree[0].children[0].review_count == 5 # child
assert tree[0].children[0].review_count == 10 # child

# .counts() should match
col.decks.select(child["id"])
col.sched.reset()
assert col.sched.counts() == (0, 0, 5)
assert col.sched.counts() == (0, 0, 10)

# answering a card in the child should decrement parent count
c = col.sched.getCard()
col.sched.answerCard(c, 3)
assert col.sched.counts() == (0, 0, 4)
assert col.sched.counts() == (0, 0, 9)

tree = col.sched.deck_due_tree().children
assert tree[0].review_count == 4 # parent
assert tree[0].children[0].review_count == 4 # child
assert tree[0].children[0].review_count == 9 # child


def test_button_spacing():
Expand Down
63 changes: 56 additions & 7 deletions rslib/src/decks/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use super::{Deck, DeckKind, DueCounts};
use crate::{
backend_proto::DeckTreeNode,
collection::Collection,
config::SchedulerVersion,
deckconf::{DeckConf, DeckConfID},
decks::DeckID,
err::Result,
Expand Down Expand Up @@ -122,6 +123,44 @@ fn apply_limits(
node.review_count = (node.review_count + child_rev_total).min(remaining_rev);
}

/// Apply parent new limits to children, and add child counts to parents.
/// Unlike v1, reviews are not capped by their parents, and we return the
/// uncapped review amount to add to the parent. This is a bit of a hack, and
/// just tides us over until the v2 queue building code can be reworked.
/// Counts are (new, review).
fn apply_limits_v2(
node: &mut DeckTreeNode,
today: u32,
decks: &HashMap<DeckID, Deck>,
dconf: &HashMap<DeckConfID, DeckConf>,
parent_limits: (u32, u32),
) -> u32 {
let original_rev_count = node.review_count;

let (mut remaining_new, remaining_rev) =
remaining_counts_for_deck(DeckID(node.deck_id), today, decks, dconf);

// cap remaining to parent limits
remaining_new = remaining_new.min(parent_limits.0);

// apply our limit to children and tally their counts
let mut child_new_total = 0;
let mut child_rev_total = 0;
for child in &mut node.children {
child_rev_total +=
apply_limits_v2(child, today, decks, dconf, (remaining_new, remaining_rev));
child_new_total += child.new_count;
// no limit on learning cards
node.learn_count += child.learn_count;
}

// add child counts to our count, capped to remaining limit
node.new_count = (node.new_count + child_new_total).min(remaining_new);
node.review_count = (node.review_count + child_rev_total).min(remaining_rev);

original_rev_count + child_rev_total
}

fn remaining_counts_for_deck(
did: DeckID,
today: u32,
Expand Down Expand Up @@ -251,13 +290,23 @@ impl Collection {
.map(|d| (d.id, d))
.collect();
add_counts(&mut tree, &counts);
apply_limits(
&mut tree,
days_elapsed,
&decks_map,
&dconf,
(std::u32::MAX, std::u32::MAX),
);
if self.sched_ver() == SchedulerVersion::V2 {
apply_limits_v2(
&mut tree,
days_elapsed,
&decks_map,
&dconf,
(std::u32::MAX, std::u32::MAX),
);
} else {
apply_limits(
&mut tree,
days_elapsed,
&decks_map,
&dconf,
(std::u32::MAX, std::u32::MAX),
);
}
}

Ok(tree)
Expand Down