Hi, I think there may be a bug in fit_fast related to the default value of model_type.
In the function signature I see:
def fit_fast(
features,
targets,
model_type="restrospective",
...
):
but later in the code the checks are against "retrospective":
if model_type=="retrospective":
...
elif model_type=="prospective":
...
elif model_type=="prospective-restricted:":
...
else:
...
Because of this, calling fit_fast(...) without explicitly passing model_type seems to use "restrospective" (with typo), which does not match "retrospective" in the downstream conditionals.
This appears to change the behavior materially. In my case:
SparseSC.fit_fast(
features=features,
targets=targets,
treated_units=treated_units,
)
gives different results from:
SparseSC.fit_fast(
features=features,
targets=targets,
treated_units=treated_units,
model_type="retrospective",
)
From reading the code, it looks like the default typo causes execution to fall through to the else branch instead of the intended "retrospective" branch.
There may also be another typo here:
elif model_type=="prospective-restricted:":
with a trailing : inside the string.
So I think there are possibly two string-matching issues:
"restrospective" instead of "retrospective" in the default argument
"prospective-restricted:" instead of "prospective-restricted"
Expected behavior:
- the default
model_type should likely be "retrospective"
- calling
fit_fast(...) with no explicit model_type should behave the same as fit_fast(..., model_type="retrospective")
Thanks for taking a look.
Hi, I think there may be a bug in
fit_fastrelated to the default value ofmodel_type.In the function signature I see:
but later in the code the checks are against
"retrospective":Because of this, calling
fit_fast(...)without explicitly passingmodel_typeseems to use"restrospective"(with typo), which does not match"retrospective"in the downstream conditionals.This appears to change the behavior materially. In my case:
gives different results from:
From reading the code, it looks like the default typo causes execution to fall through to the
elsebranch instead of the intended"retrospective"branch.There may also be another typo here:
with a trailing
:inside the string.So I think there are possibly two string-matching issues:
"restrospective"instead of"retrospective"in the default argument"prospective-restricted:"instead of"prospective-restricted"Expected behavior:
model_typeshould likely be"retrospective"fit_fast(...)with no explicitmodel_typeshould behave the same asfit_fast(..., model_type="retrospective")Thanks for taking a look.