-
-
Notifications
You must be signed in to change notification settings - Fork 26k
Support cross 32bit/64bit pickles for decision tree #21552
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
Support cross 32bit/64bit pickles for decision tree #21552
Conversation
1d67599
to
298ec95
Compare
66d4660
to
808662b
Compare
…now handled by astype in __cinit__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lesteve, here is a first pass:
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…nto cross-32bit-64bit-tree-pickle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @lesteve for this epic PR :)
My comment are mostly nitpicky naming suggestions that you should feel be free to ignore (but I find my names more descriptive ;) and a couple more simple comments.
Maybe @rth is interested in giving this PR a final review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough work! LGTM as well.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
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.