Skip to content
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

Fix: Inaccurate Attribute Listing with dir(obj) #28749

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MiguelParece
Copy link

@MiguelParece MiguelParece commented Apr 2, 2024

Inaccurate Attribute Listing with__dir__(obj) in classes with conditional methods, I overwrote the python method dir() in all the classes that contain conditional methods

Fixes #28558

Copy link

github-actions bot commented Apr 2, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6e48d35. Link to the linter CI: here

@betatim betatim changed the title Fix: Inaccurate Attribute Listing with dir(obj) (scikit-learn#28558) Fix: Inaccurate Attribute Listing with dir(obj) Apr 3, 2024
@betatim
Copy link
Member

betatim commented Apr 3, 2024

Thanks for tackling this and making your first contribution to scikit-learn!

Some thoughts on what to do next:

  • add a test to make sure that if a class as a conditional method it also has a modified __dir__. And that the reverse is also true: if there are no conditional methods, there should be no __dir__ defined
    • the tricky bit here is to write the test as a common test so that we don't have to make a list of classes
  • add a test to check that the result of calling __dir__ is correct, for all possible conditions of available_if
  • add a comment to the __dir__ implementations that explains why it is there
  • no (deprecation) warnings are raised when triggering attribute listing
    • deprecated attributes should be listed
  • bonus: find a way to reduce the need for defining __dir__ everywhere, but maybe this isn't possible

@lesteve
Copy link
Member

lesteve commented Apr 4, 2024

Not sure what is causing the CI error, I guess your __dir__ implementation iterates over all the attributes and raises a warning for deprecated attributes somehow. Not sure why it works in one CI build and not the other ...

Maybe try to debug this, and maybe catching the warnings inside __dir__ could be a stop-gap solution, at least for now and we can rediscuss how to do this in a better way when we are closer to review this in more details.

@MiguelParece
Copy link
Author

@betatim all good ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate Attribute Listing with dir(obj) for Classes Using available_if Conditional Method Decorator
3 participants