-
Notifications
You must be signed in to change notification settings - Fork 28
[ENH] extend all_objects tag_filter to allow str or list of str for filtering
#422
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
base: main
Are you sure you want to change the base?
Conversation
…ly handles 'filter_tags' as a dict
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!
Also, please do not change the logic of the existing tests. Only add tests.
Changing the error message to match the argument is of course ok.
|
Hello! Please let me know your review on them. |
all_objects tag_filter to allow str or list of str for filtering
| if tag_filter is None: | ||
| return True | ||
|
|
||
| type_msg = ( |
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.
it looks like the function already allowed for iterable of str or str
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.
True, tho! The function already had full support for both single strings and iterables of strings.
I did add redundant but more granular and edge case tests of the existing functionality; already implemented and working.
Should I remove those new tests I added?
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.
it seems like you are still changing the logic itself, making it less general, and removing the useful error message.
fkiraly
left a comment
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.
Reviewing the code, it appears that the current all_objects already allowed the user to pass str or listof str. This was only not reflected in the docstring.
Can you please check whether the changes to all_objects were necessary?
Sorry for not noticing this earlier.
The tests are useful still, though.
|
Hello @fkiraly ! |
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!
It seems like you are still changing the logic itself, making it less general, and removing the useful error message.
Is this really necessary? Please comment.
[ENH] Add filter_tags preprocessing to accept strings and lists in all_objects and get_package_metadata
Reference Issues
Fixes #8210 from sktime
Changes:
This PR enhances the all_objects and get_package_metadata functions by adding preprocessing logic for the filter_tags parameter. Previously, these functions only handled filter_tags as a dict. Now they accept more user-friendly input formats:
i) String input:
filter_tags="A"→ converts to{"A": True}ii) List/tuple input:
filter_tags=["A", "B"]→ converts to{"A": True, "B": True}iii) Dict input: Existing functionality preserved (dict is copied to prevent modification)
iv) Error handling: Clear TypeError for invalid input types
What should a reviewer concentrate their feedback on?