-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
Conversation
@@ -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) |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
|
||
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])) |
There was a problem hiding this comment.
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 ¯\_(ツ)_/¯
There was a problem hiding this comment.
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 |
closing b/c the problem is better addressed by #1966 |
Description
A possible source of problems in
doctor_visits
is Weekday adjustment. This adds warnings if the parameters ever get too large, and overrides them if they're ever infinite.Changelog
Itemize code/test/documentation changes and files added/removed.
exp(-params)
appropriatelyFixes
doctor_visits
or not