Skip to content

Return expected number of values in compute_prebins for ContinuousOptimalBinning#358

Merged
guillermo-navas-palencia merged 3 commits into
guillermo-navas-palencia:developfrom
YC-1412:update-compute-prebin-returns
Jun 3, 2025
Merged

Return expected number of values in compute_prebins for ContinuousOptimalBinning#358
guillermo-navas-palencia merged 3 commits into
guillermo-navas-palencia:developfrom
YC-1412:update-compute-prebin-returns

Conversation

@YC-1412

@YC-1412 YC-1412 commented May 28, 2025

Copy link
Copy Markdown
Contributor

Fixes #357.

Please see the context in #357. This PR make _compute_prebins in ContinuousOptimalBinning always return 8 values. Missing values are filled with empty numpy array.

I used the test code in #357 to test the fix.

Before
image

After
image

@YC-1412

YC-1412 commented May 30, 2025

Copy link
Copy Markdown
Contributor Author

Hi @guillermo-navas-palencia, I noticed the pytest workflow failed on test_binning_piecewise.py. It seems to come from the osqp version (one dependency ofcvxpy). When I test it locally, it fails on the main branch code as well.

When I use osqp==0.6.7.post2, the test passes, but it fails when osqp>1.0. It produces a different optimizer.coef_ from theRobustPWRegression optimizer while the input x and y are unchanged.

  • optimizer.coef_:

    optimizer = RobustPWRegression(
    objective=self.objective,
    degree=self.degree,
    continuous=self.continuous,
    continuous_deriv=self.continuous_deriv,
    monotonic_trend=monotonic,
    solver=self.solver,
    h_epsilon=self.h_epsilon,
    quantile=self.quantile,
    regularization=self.regularization,
    reg_l1=self.reg_l1,
    reg_l2=self.reg_l2,
    extrapolation="continue",
    verbose=self.verbose)
    optimizer.fit(x_subsamples, pred_subsamples, splits, lb=lb, ub=ub)
    self._c = optimizer.coef_

    image

  • test result:
    image

Unfortunately, I'm not sure about the root cause and version difference in osqp. The results look close to me so it might be some rounding update. It passes the test if setting the tolerance to $10^{-5}$ or updating the expected value to the result of the newer version (5.84465065). Please let me know if you think one of them sounds a reasonable fix.

@guillermo-navas-palencia

Copy link
Copy Markdown
Owner

Hi @YC-1412. I think setting a tolerance of 1e-4 is enough. I have had similar problems with previous cvxpy updates/changes.

@YC-1412

YC-1412 commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Hi @guillermo-navas-palencia, thanks for the confirmation. I took the liberty to update the tolerance in this PR. If it's not appropriate, I can revert it. Thanks!

@guillermo-navas-palencia

Copy link
Copy Markdown
Owner

The tolerance change for 1 test was missing. I think after this change, all tests will pass :)

@YC-1412

YC-1412 commented Jun 3, 2025

Copy link
Copy Markdown
Contributor Author

Updated! Thank you!

@guillermo-navas-palencia guillermo-navas-palencia merged commit da450a1 into guillermo-navas-palencia:develop Jun 3, 2025
12 checks passed
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