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

ELEX-4413 update dependencies and switch to using the Clarabel solver #22

Conversation

dmnapolitano
Copy link
Contributor

@dmnapolitano dmnapolitano commented Sep 12, 2024

Description

Hi! The changes in this PR update elex-solver's dependencies to their latest versions and silences a warning produced by cvxpy about the fact that we're still using the ECOS solver switches QuantileRegressionSolver's cvxpy solver from ECOS to Clarabel 🎉

Switching from ECOS to cvxpy's recommendation of CLARABEL is pretty easy and will probably result in absolutely no changes in terms of the predictions produced by our model, but IMO that's something we'd want to test pretty extensively before making the switch 😬 So I'm silencing the warning here per cvxpy's recommendation.

Jira Ticket

ELEX-4413

Test Steps

tox and/or pip install -e . and run the elexmodel and/or test bed commands of your choice 🎉

@dmnapolitano dmnapolitano requested a review from a team as a code owner September 12, 2024 15:28
@lennybronner
Copy link
Collaborator

oh interesting, do you know why they are switching away from ecos?

@dmnapolitano
Copy link
Contributor Author

oh interesting, do you know why they are switching away from ecos?

Here's what I found: https://www.cvxpy.org/updates/#ecos-deprecation

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.

🤔

@lennybronner
Copy link
Collaborator

ok, so I think we should just go for it with clarabel 👀

@dmnapolitano
Copy link
Contributor Author

ok, so I think we should just go for it with clarabel 👀

You know, I was just thinking the same thing 😂 🎉

@dmnapolitano dmnapolitano marked this pull request as draft September 12, 2024 15:37
@dmnapolitano dmnapolitano marked this pull request as ready for review September 12, 2024 15:41
@dmnapolitano
Copy link
Contributor Author

ok, so I think we should just go for it with clarabel 👀

You know, I was just thinking the same thing 😂 🎉

@lennybronner switched! 🎉

@dmnapolitano dmnapolitano changed the title ELEX-4413 update dependencies and explicitly specify cvxpy solver ELEX-4413 update dependencies and ~~explicitly specify cvxpy solver~~ switch to using the Clarabel solver Sep 12, 2024
@dmnapolitano dmnapolitano changed the title ELEX-4413 update dependencies and ~~explicitly specify cvxpy solver~~ switch to using the Clarabel solver ELEX-4413 update dependencies and switch to using the Clarabel solver Sep 12, 2024
Copy link
Collaborator

@lennybronner lennybronner left a comment

Choose a reason for hiding this comment

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

did you run this locally with the model?

@dmnapolitano
Copy link
Contributor Author

did you run this locally with the model?

Yup! 🎉 Although, I haven't done a run with the testbed yet, and I do have some results I could compare to...let me see 🤔

@lennybronner
Copy link
Collaborator

Also when you run this, just confirm that you are using the cvxpy solver. that only gets used when we run quantile regression with regularization.

@dmnapolitano
Copy link
Contributor Author

dmnapolitano commented Sep 12, 2024

Also when you run this, just confirm that you are using the cvxpy solver. that only gets used when we run quantile regression with regularization.

Oh, right! 🤦🏻‍♀️ Ok, in that case, here's something interesting. This command runs just fine:

elexmodel 2023-11-07_VA_G --estimands=margin --features=baseline_normalized_margin --office_id=Y --geographic_unit_type=county-district --pi_method bootstrap

Now if I want to test the regularization, I switch to pi_method=gaussian (with model_parameters='{"lambda" : whatever}', but):

elexmodel 2023-11-07_VA_G --estimands=margin --features=baseline_normalized_margin --office_id=Y --geographic_unit_type=county-district --pi_method gaussian

And this second command throws a ValueError: Array contains NaN or Infinity exception 🤔 🤔

@lennybronner
Copy link
Collaborator

lennybronner commented Sep 12, 2024

right, margin only works with the bootstrap model

@dmnapolitano
Copy link
Contributor Author

right, margin only works with the bootstrap model

OMG 🙈 Wait, why is that? 🤔

@lennybronner
Copy link
Collaborator

because margin necessitates a whole bunch of math that we never implemented for the other models

@lennybronner
Copy link
Collaborator

specifically, margin needs moving from normalized to unnormalized margin space a few times that we haven't implemented in the other models (to go from unit margins to aggregate margins for example). But specifically, this error suggests that a unit is coming through to the model that shouldn't be (we should be converting all margins from nan to zero). might be worth investigating.

@dmnapolitano
Copy link
Contributor Author

specifically, margin needs moving from normalized to unnormalized margin space a few times that we haven't implemented in the other models (to go from unit margins to aggregate margins for example). But specifically, this error suggests that a unit is coming through to the model that shouldn't be (we should be converting all margins from nan to zero). might be worth investigating.

Oh wow! Yeah, that does sound possible. At some point in the post-GE future lol 🤔

Ok, in that case...how much testing is worth doing here? I can (now) get this to run fine:

elexmodel 2023-11-07_VA_G --estimands=dem --features=median_household_income --office_id=Y --geographic_unit_type=county-district --pi_method gaussian

Adding in regularization works but makes no difference in the output 🤔 Should I run some testbed commands with the current elex-solver vs. this with regularization, just to see? 🤔

Copy link
Collaborator

@lennybronner lennybronner left a comment

Choose a reason for hiding this comment

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

nope, this is good enough!

@dmnapolitano dmnapolitano merged commit 0676a3c into develop Sep 12, 2024
3 checks passed
@dmnapolitano dmnapolitano deleted the ELEX-4413-update-dependencies-and-explicitly-specify-CVXPY-solver branch September 12, 2024 20:45
This was referenced Sep 12, 2024
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.

2 participants