Skip to content

MNT fixed test for QuantileRegressor#374

Merged
BenjaminBossan merged 2 commits intoskops-dev:mainfrom
EdAbati:fix-quantile-regressor
Jun 27, 2023
Merged

MNT fixed test for QuantileRegressor#374
BenjaminBossan merged 2 commits intoskops-dev:mainfrom
EdAbati:fix-quantile-regressor

Conversation

@EdAbati
Copy link
Copy Markdown
Contributor

@EdAbati EdAbati commented Jun 26, 2023

What does this implement/fix? Explain your changes.

Some tests are failing because the default solver="interior-point" of QuantileRegressor is not available in scipy>=1.11.0. Please see these tests as an example.
scikit-learn>=1.4.0 will change the default. In the meantime, this PR should fix the issue .

Copy link
Copy Markdown
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Besides a small typo, this LGTM. Thanks a lot for your initiative Edoardo.

The failing tests seem to be unrelated, so this can be merged from my POV.

What I do wonder though: Does this mean that using the default solver for QuantileRegressor raises an error? That seems pretty bad. Ping @adrinjalali

Comment thread skops/io/tests/test_persist.py Outdated
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@BenjaminBossan
Copy link
Copy Markdown
Collaborator

The error that occurs is very strange, as it seems like an outdated sklearn version is being used. I could reproduce the error locally yesterday, but cannot do so today (new sklearn nightly release since then). I therefore assume the error is gone and it's a caching issue on GH or something. Anyway, the error is not related to this PR so it can be merged.

@BenjaminBossan BenjaminBossan merged commit 107904c into skops-dev:main Jun 27, 2023
@EdAbati EdAbati deleted the fix-quantile-regressor branch June 27, 2023 12:41
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