-
Notifications
You must be signed in to change notification settings - Fork 514
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
add feature_names_in_ #5678
base: branch-24.02
Are you sure you want to change the base?
add feature_names_in_ #5678
Conversation
/ok to test |
@csadorf can you run the build? |
/ok to test |
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.
Tests are currently failing because of a missing import. I would appreciate if you could fix that before I do a more thorough review.
@tvdboom Let me know if there is any way I can assist you with this PR! |
I haven't found the time to work on this yet. I'll do it for sure next week. thanks |
Sure, no rush at all! Just let me know when you want to move forward with this. |
@csadorf It should be ready now. I imported numpy over cupy. I believe for this specific array that's probably faster but let me know if you think cupy would be the better choice here. |
/ok to test |
/ok to test |
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.
Looks good overall, thanks a lot!
I ran some tests and also noticed that the warnings
module is used in the skl_dependencies.py
module, but is not actually imported. That would of course need to be fixed, but I'm also a bit concerned, because it means that those branches are not actually tested. I would appreciate if you could have a look at how much effort it would take to test those cases.
/ok to test |
@tvdboom Please do not merge the remote tracking branch unless necessary. It stresses our CI resources. Also, please install the pre-commit hooks to ensure that style checks will pass. Thanks! |
/ok to test |
@tvdboom Looking at the test outputs, it seems like it's not working as expected. Do you have any idea what might be going on? Do you need assistance in setting up a local test environment for debugging? |
Unfortunately, I have a new pc with an amd gpu and windows os so I can't install cuml locally. So what I did was to run the tests importing a model from sklearn instead of cuml and (naively) hope that this would also work with the same model from cuml. Apparently it does not, so not sure how to debug. |
That's unfortunate. Maybe you can use Google Colab? |
Any idea how to install cuml from a custom branch in colab? I tried this: Also adding |
Closes #5677
Adds the
feature_names_in_
attributes to all estimators. Code copied from sklearn's BaseEstimator.