-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST Add TODO for global_dtype in sklearn/ensemble/tests/test_forest.py #22941
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
Conversation
Hi @adam2392, I am following up with this PR which I forgot. I do not have commit rights on @neurodata's fork of scikit-learn. |
Done! |
I think it is worth adding float32 support in tree based models before working further on extending tests as data is cast to float64 at the moment: |
What's the difficulty here? Could we just re-define the trees and sub the data types for a fused datatype? If so, I think I can submit a straightforward PR. Lmk if you think this is insufficient tho. E.g.
|
Unfortunately fused-types and Cython extension types can't be used simultaneously. More information and resolution pathways are given here: #7059 (comment) |
Oh yeah forgot bout that unfortunate limitation :p |
Things would significantly be simpler for scikit-learn if Cython were to support it. IIRC, supporting this really is complicated because one has to deal with many historical aspects and choices of Pyrex which Cython supersedes. Edit: cython/cython#3283 tracks discussions on this topic. |
Reference Issues/PRs
See: #22881 (comment)
What does this implement/fix? Explain your changes.
Adds an inline comment for TODO behavior change for global_dtype tests in ensemble trees in the future.
Any other comments?
cc: @jjerphan