Skip to content

add warnings for unreasonable weekday effect #1964

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

add warnings for unreasonable weekday effect #1964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2890b3
add warnings for unreasonable weekday effect
dsweber2 May 22, 2024
7b5a4ea
include Sunday and warn if big but finite
dsweber2 May 22, 2024
b36be28
delphi-utils version bump
dsweber2 May 22, 2024
b39a540
version unbump
dsweber2 May 23, 2024
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
20 changes: 15 additions & 5 deletions _delphi_utils_python/delphi_utils/weekday.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ def get_params(data, denominator_col, numerator_cols, date_col, scales, logger):
series column in the data.
"""
tmp = data.reset_index()
denoms = tmp.groupby(date_col).sum()[denominator_col]
nums = tmp.groupby(date_col).sum()[numerator_cols]

denoms = tmp.groupby(date_col).sum(numeric_only = True)[denominator_col]
nums = tmp.groupby(date_col).sum(numeric_only = True)[numerator_cols]
if nums.shape[0] < 7:
logger.warning("Trying to handle weekday effects with fewer than 7 days worth of data. This will probably not work.")
# Construct design matrix to have weekday indicator columns and then day
# indicators.
X = np.zeros((nums.shape[0], 6 + nums.shape[0]))
Expand All @@ -40,7 +41,15 @@ def get_params(data, denominator_col, numerator_cols, date_col, scales, logger):
logger.error("Unable to calculate weekday correction")
else:
params[i,:] = result

# the values multiplied by in calc_adjustment
# Sunday, the last day here, is a sum of the other days
effective_multipliers = np.exp(np.concatenate([-params[:,:6], params[:,:6].sum(axis=0, keepdims = True)]))

if effective_multipliers.max() == np.inf or effective_multipliers.min() == -np.inf:
logger.warning("largest weekday correction is infinite. Defaulting to no correction")
params = np.zeros((nums.shape[1], X.shape[1]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe make this replacement optional? itd be nice if we could bubble up to the caller that this has happened, but im not sure how to do that nicely without throwing an exception (which defeats the purpose) or by changing the signature on the return (which might break some uses of this class)... perhaps with a "pass by reference"-style argument that this method can modify? thats kinda uglor but ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case, the values are literally impossible to use, since it's multiplying by infinity. I suppose it should probably just kill the whole process.

note that passing all zeros is interpreted as a failure by doctor-visits, see here:

covidcast-indicators/doctor_visits/delphi_doctor_visits/update_sensor.py

Lines 136 to 138 in 0c65bb4

if weekday and np.any(np.all(params == 0,axis=1)):
# Weekday correction failed for at least one count type
return None

elif effective_multipliers.max() > 1000:
logger.warning("largest weekday correction is over a factor of 1000. This is probably an error, but may be associated with a zero value.")
return params

@staticmethod
Expand Down Expand Up @@ -93,11 +102,12 @@ def _fit(X, scales, npnums, npdenoms):
for scale in scales:
try:
prob = cp.Problem(cp.Minimize((-ll + lmbda * penalty) / scale))
_ = prob.solve()
_ = prob.solve(solver = cp.ECOS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the default solver? i presume youre just making the choice explicit...

Copy link
Contributor Author

@dsweber2 dsweber2 May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was the default until recently. This code is still using the old default, and I'm including the explicit restriction to maintain the same behavior whenever we eventually migrate to a newer python version. (ECOS isn't the source of the problems, when I was running locally it was using ECOS as wel).

return b.value
except SolverError:
# If the magnitude of the objective function is too large, an error is
# thrown; Rescale the objective function by going through loop
print(f"Solver didn't work at {scale}x")
continue
return None

Expand Down
14 changes: 9 additions & 5 deletions doctor_visits/delphi_doctor_visits/download_claims_ftp_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,16 @@ def download(ftp_credentials, out_path, logger):
client = paramiko.SSHClient()
client.set_missing_host_key_policy(AllowAnythingPolicy())

client.connect(ftp_credentials["host"],
username=ftp_credentials["user"],
password=ftp_credentials["pass"],
port=ftp_credentials["port"])
client.connect(
ftp_credentials["host"],
username=ftp_credentials["user"],
password=ftp_credentials["pass"],
port=ftp_credentials["port"],
allow_agent=False,
look_for_keys=False,
)
sftp = client.open_sftp()
sftp.chdir('/optum/receiving')
sftp.chdir("/optum/receiving")

# go through files in recieving dir
files_to_download = []
Expand Down
Loading