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

Weekday adjustment to use Clarabel instead of ECOS #1966

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

minhkhul
Copy link
Contributor

@minhkhul minhkhul commented May 30, 2024

Description

doctor_visits signal smoothed_adj_cli has a bug where sometimes some percentage values are calculated to be abnormally big (90%+). We found out it's because of this ECOS solver used by default by cvxpy when calculating weekday adjustments.

Per their recent update:

CVXPY has used ECOS as the default solver for many years; however, it has known issues with performance and numerical stability in edge cases. Recently, a new solver, Clarabel, that improves the algorithm and implementation of ECOS has been under development.

Changelog

  • Force weekday adjustment to use Clarabel in all versions of cvxpy, not just 1.5+.

Fixes

  • Annoying doctor_visits bug

@melange396
Copy link
Contributor

Since the python client bug is fixed, i reran the test actions. Now test_weekday.py is failing because the expected output values have changed slightly.

@dshemetov
Copy link
Contributor

You can cherrypick this commit if you want to fix just that CI issue 3559664. It's included in the #1952 and (the same commit with a different hash is in) #1905.

@melange396
Copy link
Contributor

weird! @dshemetov do you know what caused that?

@minhkhul
Copy link
Contributor Author

minhkhul commented May 31, 2024

@melange396 I'm gonna guess it's related to clarabel solver being made default recently in cvxpy instead of ecos. That test ran through the exact same code I'm making changes to.

@dshemetov
Copy link
Contributor

dshemetov commented May 31, 2024

Yea, it's coming from differences in the solvers. Clarabel is a new Rust/Julia optimization library that supersedes ECOS, the previously default C optimizer, with more performant algorithms. Some details in this changelog.

@melange396
Copy link
Contributor

ah yes, the default changed with the new version, and it bit them in those other PRs not long after the release. This is an argument for using version pinning... I will create a GH issue for that.

Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

Yep, glad to see this! Strange that they differ even in the 4th digit; I would've expected them to converge to 1e-8

@melange396
Copy link
Contributor

Do we want to do more comparisons to try to better understand the impact of this change? At the very least, we will want to document any signals affected by this solver substitution and when it is actually applied to the data. Beyond just doctor_visits, it looks like perhaps just claims_hosp (and not changehc, since that is no longer active).

@dsweber2
Copy link
Contributor

dsweber2 commented Jun 3, 2024

That would make sense; it's a thing to do after this has gone live and been run on the old data though, right? Added as a subtask on the Jira patch ticket

Judging by the effect on the test case, it should cause changes on the order of ~.1%, which is probably not worth worrying about too much. If anything the values should be closer to the true optimum.

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 this pull request may close these issues.

4 participants