Support cross 32bit/64bit pickles for decision tree#21552
Merged
rth merged 24 commits intoscikit-learn:mainfrom Nov 25, 2021
Merged
Support cross 32bit/64bit pickles for decision tree#21552rth merged 24 commits intoscikit-learn:mainfrom
rth merged 24 commits intoscikit-learn:mainfrom
Conversation
This was referenced Nov 4, 2021
1d67599 to
298ec95
Compare
66d4660 to
808662b
Compare
…now handled by astype in __cinit__
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…nto cross-32bit-64bit-tree-pickle
ogrisel
reviewed
Nov 18, 2021
jjerphan
reviewed
Nov 19, 2021
Member
jjerphan
left a comment
There was a problem hiding this comment.
A first shallow review on this IMO support.
Current tests LGTM. 👍
I think that I still need to understand the details and associated challenges of cross 32 and 64 bit support to fully help with this PR.
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
…nto cross-32bit-64bit-tree-pickle
Member
|
Maybe @rth is interested in giving this PR a final review? |
ogrisel
reviewed
Nov 25, 2021
rth
approved these changes
Nov 25, 2021
Member
rth
left a comment
There was a problem hiding this comment.
Thanks for the thorough work! LGTM as well.
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Nov 29, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
samronsin
pushed a commit
to samronsin/scikit-learn
that referenced
this pull request
Nov 30, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Dec 24, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre
pushed a commit
that referenced
this pull request
Dec 25, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Fix #19602.
What does this implement/fix? Explain your changes.
This makes it possible to save a decision tree pickle on a 64bit machine and load it on a 32bit machine (as well as the other way around although this is probably not that useful in practice).
The test uses a similar approach as #21539: it checks that a pickle containing arrays of a different bitness (e.g. 64bit arrays on a 32bit machine) can be loaded fine. This is not exactly what we want to test, i.e. generating a pickle on 64bit machine and load it on a 32 bit machine but is close enough.
If we want, we could have an additional test that is exactly the use case we are aiming to support. We can generate pickle bytes on a 64 bit machine (we can put a comment indicating how to generate it if we need to regenerate it) and load it on the 32 bit CI.
Any other comments?
#21539 should be merged first, putting this one in draft for now.
Testing a similar thing for more/other estimators is left for a future PR.