FIX Deep copy criterion in trees to fix concurrency bug #19580
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3e034f1 to
fc7ce52
Compare
|
ping @thomasjpfan @glemaitre |
fc7ce52 to
9157002
Compare
9157002 to
75429ea
Compare
|
Could you please add a changelog entry in |
66491e9 to
3ff8dc2
Compare
|
Ok, moved changelog entry from v1.0 to v0.24.2 |
thomasjpfan
reviewed
Mar 1, 2021
There was a problem hiding this comment.
Thank you for the PR @samdbrice.
Minor suggestion, otherwise LGTM!
3ff8dc2 to
1abb39d
Compare
|
Reused the pre-generated data but had to reshape |
thomasjpfan
reviewed
Mar 1, 2021
…nt segfault caused by concurrent accesses.
1abb39d to
48c45c5
Compare
|
Ok, all set. Let me know if there's anything else |
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Apr 22, 2021
…#19580) Co-authored-by: Samuel Brice <samuel.brice@twosigma.com>
glemaitre
pushed a commit
that referenced
this pull request
Apr 28, 2021
Co-authored-by: Samuel Brice <samuel.brice@twosigma.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Fixes #12623
Adds a smoke test as requested in #12690
Implements deep copy within tree.BaseDecisionTree.fit as requested in #18985
What does this implement/fix? Explain your changes.
The issue is that when trees are created by the base ensemble class, the parameters are not copied. This is problematic in the case of the criterion parameter if a
Criterionobject is passed in. When this happens, all the trees share the same object and mutate it. Ifn_jobs > 1(or-1, and we have more than one core available), they are mutating the same object concurrently.Any other comments?
The functional changes of this PR are the same as those in #18985. The deep copy has been moved to within.
BaseDecisionTree.fitas to be more targeted and prevent possibly costly object deep copies withinBaseEnsemble._make_estimator.