Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix check_decision_proba_consistency random failure #19225

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

Merged

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Jan 21, 2021

Tentative fix for #19224.

  • I noticed that X_test was generated from the RNG singleton of np.random and therefore the test was not deterministic.
  • I now generate X_test from the same bloby distribution as the training set which is less likely to result in samples close to the decision boundary although I am not sure this has any actual impact. But it seems more natural to do so.
  • I removed the rounding thingy that I did not understand. How rounding could possibly reduce the likelihood of ties?

Anyway lets see how the CI likes this including the [cd build].

@ogrisel
Copy link
Member Author

ogrisel commented Jan 21, 2021

I removed the rounding thingy that I did not understand. How rounding could possibly reduce the likelihood of ties?

Apparently it was necessary for the Gradient Boosting classifiers on 32 bit linux... So apparently the goal is to create deterministic ties rather than close but non-deterministic non-ties at the rounding error level. I will revert this part of the change.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

Side note: Rounding to support different platforms may become a pattern: #19221

@ogrisel ogrisel changed the title Fix check decision proba consistency Fix check_decision_proba_consistency random failure Jan 21, 2021
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@ogrisel ogrisel added the module:test-suite everything related to our tests label Jan 21, 2021
Base automatically changed from master to main January 22, 2021 10:53
@adrinjalali adrinjalali merged commit 0f0eb52 into scikit-learn:main Jan 23, 2021
@glemaitre glemaitre added this to the 0.24.2 milestone Feb 11, 2021
@glemaitre glemaitre added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Feb 11, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
* FIX more deterministic check_decision_proba_consistency

* Trigger [cd build]

* Re-add rounding

* Trigger [cd build]

* Avoid redundant phrasing in comment [ci skip]
glemaitre pushed a commit that referenced this pull request Apr 28, 2021
* FIX more deterministic check_decision_proba_consistency

* Trigger [cd build]

* Re-add rounding

* Trigger [cd build]

* Avoid redundant phrasing in comment [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI module:test-suite everything related to our tests module:utils To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.