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

[BUG] The same-day reviews are filtered out when passing --secs to other.py. #158

Closed
L-M-Sherlock opened this issue Jan 19, 2025 · 6 comments

Comments

@L-M-Sherlock
Copy link
Member

After #152, when I run python other.py --model FSRS-4.5 --secs, the same-day reviews are filtered out. I located the problematic code:

srs-benchmark/other.py

Lines 2472 to 2474 in 3993541

if not SHORT_TERM:
df.drop(df[df["elapsed_days"] == 0].index, inplace=True)
df["i"] = df.groupby("card_id").cumcount() + 1

The condition should be if not secs_ivl and not SHORT_TERM. But it will induce another error when I run python other.py --model FSRS-4.5 --secs --equalize_test_with_non_secs:

Traceback (most recent call last):                                                                   
  File "/Users/jarrettye/Codes/open-spaced-repetition/fsrs-benchmark/utils.py", line 12, in wrapper
    return func(*args, **kwargs), None
  File "/Users/jarrettye/Codes/open-spaced-repetition/fsrs-benchmark/other.py", line 2733, in process
    dataset = create_features(df_revlogs, MODEL_NAME)
  File "/Users/jarrettye/Codes/open-spaced-repetition/fsrs-benchmark/other.py", line 2662, in create_features
    assert np.equal(df_intersect["i"], df_non_secs["i"]).all()
AssertionError

Could you help me? @1DWalker

@L-M-Sherlock
Copy link
Member Author

@1DWalker, I found another problem: the "delta_t" is not secs even when the flag of --secs is passed.

@1DWalker
Copy link
Contributor

Isn't it intended behaviour since --short was not used?
--short: use same-day reviews
--secs: use seconds
--secs + --short: use seconds for all reviews and use same-day reviews

@1DWalker
Copy link
Contributor

I will later try to reproduce the delta_t issue.

@L-M-Sherlock
Copy link
Member Author

I will later try to reproduce the delta_t issue.

Here is the problem:

srs-benchmark/other.py

Lines 2493 to 2505 in 3993541

if EQUALIZE_TEST_WITH_NON_SECS:
df["t_history"] = [
",".join(map(str, item[:-1])) for sublist in t_history_non_secs for item in sublist
]
df["t_history_secs"] = [
",".join(map(str, item[:-1])) for sublist in t_history_secs for item in sublist
]
else:
# If we do not care about test equality, we are allowed to overwrite delta_t and t_history
df["delta_t"] = df["delta_t_secs"]
df["t_history"] = [
",".join(map(str, item[:-1])) for sublist in t_history_secs for item in sublist
]

@1DWalker
Copy link
Contributor

python other.py --model FSRS-4.5 --secs I ran this command for the first user and printed "delta_t" and got something that looks like seconds but scaled to days: 0, 0.993623, 0.755822, etc. What are you expecting instead?

@L-M-Sherlock
Copy link
Member Author

Fine. I figured it out. My previous expectation is the same-day reviews are included if --secs is True. But I changed my mind.

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 a pull request may close this issue.

2 participants