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

MNT fixed test for QuantileRegressor #374

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

EdAbati
Copy link
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
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

skops/io/tests/test_persist.py Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@BenjaminBossan
Copy link
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