Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 35 additions & 22 deletions skbase/lookup/_lookup.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,34 +190,21 @@ def _filter_by_tags(obj, tag_filter=None, as_dataframe=True):
if tag_filter is None:
return True

type_msg = (

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Author

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?

Copy link
Copy Markdown
Contributor

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.

"filter_tags argument of all_objects must be "
"a dict with str or re.Pattern keys, "
"str, or iterable of str, "
"but found"
)

if not isinstance(tag_filter, (str, Iterable, dict)):
raise TypeError(f"{type_msg} type {type(tag_filter)}")
if not isinstance(tag_filter, dict):
raise TypeError(
"tag_filter argument must be a dict with str keys, "
f"but found type {type(tag_filter)}"
)

if not hasattr(obj, "get_class_tag"):
return False

# case: tag_filter is string
if isinstance(tag_filter, str):
tag_filter = {tag_filter: True}

# case: tag_filter is iterable of str but not dict
# If a iterable of strings is provided, check that all are in the returned tag_dict
if isinstance(tag_filter, Iterable) and not isinstance(tag_filter, dict):
if not all(isinstance(t, str) for t in tag_filter):
raise ValueError(f"{type_msg} {tag_filter}")
tag_filter = dict.fromkeys(tag_filter, True)

# case: tag_filter is dict
# check that all keys are str
if not all(isinstance(t, str) for t in tag_filter.keys()):
raise ValueError(f"{type_msg} {tag_filter}")
raise ValueError(
"tag_filter argument must be a dict with str keys, "
f"but found keys: {tag_filter.keys()}"
)

cond_sat = True

Expand Down Expand Up @@ -625,6 +612,19 @@ def get_package_metadata(
- "contains_base_objects": whether any module classes that
inherit from ``BaseObject``.
"""
# Handle tag_filter conversion from str or list of str to dict
if tag_filter is not None:
if isinstance(tag_filter, str):
tag_filter = {tag_filter: True}
elif isinstance(tag_filter, (list, tuple)) and all(
isinstance(tag, str) for tag in tag_filter
):
tag_filter = dict.fromkeys(tag_filter, True)
elif not isinstance(tag_filter, dict):
raise TypeError("tag_filter must be a str, list of str, or dict")
else:
tag_filter = tag_filter.copy()

module, path, loader = _determine_module_path(package_name, path)
module_info: MutableMapping = {} # of ModuleInfo type
# Get any metadata at the top-level of the provided package
Expand Down Expand Up @@ -844,6 +844,19 @@ class name if ``return_names=False`` and ``return_tags is not None``.
Modified version of ``scikit-learn``'s and sktime's ``all_estimators`` to allow
users to find ``BaseObject`` descendants in ``skbase`` and other packages.
"""
# Handle filter_tags conversion from str or list of str to dict
if filter_tags is not None:
if isinstance(filter_tags, str):
filter_tags = {filter_tags: True}
elif isinstance(filter_tags, (list, tuple)) and all(
isinstance(tag, str) for tag in filter_tags
):
filter_tags = dict.fromkeys(filter_tags, True)
elif not isinstance(filter_tags, dict):
raise TypeError("filter_tags must be a str, list of str, or dict")
else:
filter_tags = filter_tags.copy()

_, root, _ = _determine_module_path(package_name, path)
modules_to_ignore = _coerce_to_tuple(modules_to_ignore)
exclude_objects = _coerce_to_tuple(exclude_objects)
Expand Down
139 changes: 120 additions & 19 deletions skbase/lookup/tests/test_lookup.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,19 +374,13 @@ def test_filter_by_tags():
# Even if the class isn't a BaseObject
assert _filter_by_tags(NotABaseObject) is True

# Check when tag_filter is a str and present in the class
assert _filter_by_tags(ClassWithABTrue, tag_filter="A") is True
# Check when tag_filter is str and not present in the class
assert _filter_by_tags(Parent, tag_filter="A") is False
# Check when tag_filter is a dict with single tag present in the class
assert _filter_by_tags(ClassWithABTrue, tag_filter={"A": True}) is True
# Check when tag_filter is dict with tag not present in the class
assert _filter_by_tags(Parent, tag_filter={"A": True}) is False

# Test functionality when tag present and object doesn't have tag interface
assert _filter_by_tags(NotABaseObject, tag_filter="A") is False

# Test functionality where tag_filter is Iterable of str
# all tags in iterable are in the class
assert _filter_by_tags(ClassWithABTrue, ("A", "B")) is True
# Some tags in iterable are in class and others aren't
assert _filter_by_tags(ClassWithABTrue, ("A", "B", "C", "D", "E")) is False
assert _filter_by_tags(NotABaseObject, tag_filter={"A": True}) is False

# Test functionality where tag_filter is Dict[str, Any]
# All keys in dict are in tag_filter and values all match
Expand All @@ -396,17 +390,21 @@ def test_filter_by_tags():
# At least 1 key in dict is not in tag_filter
assert _filter_by_tags(Parent, {"E": 1, "B": 2}) is False

# Iterable tags should be all strings
with pytest.raises(ValueError, match=r"filter_tags"):
assert _filter_by_tags(Parent, ("A", "B", 3))
# Tags that aren't dict should raise TypeError
with pytest.raises(TypeError, match=r"tag_filter argument must be a dict"):
_filter_by_tags(Parent, "A")

with pytest.raises(TypeError, match=r"tag_filter argument must be a dict"):
_filter_by_tags(Parent, ["A", "B"])

# Tags that aren't iterable have to be strings
with pytest.raises(TypeError, match=r"filter_tags"):
assert _filter_by_tags(Parent, 7.0)
with pytest.raises(TypeError, match=r"tag_filter argument must be a dict"):
_filter_by_tags(Parent, 7.0)

# Dictionary tags should have string keys
with pytest.raises(ValueError, match=r"filter_tags"):
assert _filter_by_tags(Parent, {7: 11})
with pytest.raises(
ValueError, match=r"tag_filter argument must be a dict with str keys"
):
_filter_by_tags(Parent, {7: 11})


def test_walk_returns_expected_format(fixture_skbase_root_path):
Expand Down Expand Up @@ -998,6 +996,109 @@ def test_all_object_tag_filter(tag_filter):
assert len(unfiltered_classes) > len(filtered_classes)


def test_all_objects_filter_tags_preprocessing():
"""Test filter_tags preprocessing in all_objects function."""
# Test string input conversion
objs_str = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags="A",
)

objs_dict = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags={"A": True},
)

# Results should be identical
assert objs_str.equals(
objs_dict
), "String and dict filter should return same results"

# Test list of strings input conversion
objs_list = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags=["A", "B"],
)

objs_dict_multi = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags={"A": True, "B": True},
)

# Results should be identical
assert objs_list.equals(
objs_dict_multi
), "List and dict filter should return same results"


@pytest.mark.parametrize(
"invalid_filter",
[
123, # int
12.5, # float
object(), # object
["A", 123], # list with non-string
("A", 123), # tuple with non-string
],
)
def test_all_objects_filter_tags_invalid_types(invalid_filter):
"""Test that invalid filter_tags types raise TypeError."""
with pytest.raises(
TypeError, match="filter_tags must be a str, list of str, or dict"
):
all_objects(
package_name="skbase",
filter_tags=invalid_filter,
)


def test_all_objects_filter_tags_empty_list():
"""Test that empty list filter_tags works correctly."""
objs_empty_list = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags=[],
)

objs_empty_dict = all_objects(
package_name="skbase",
return_names=True,
as_dataframe=True,
filter_tags={},
)

# Results should be identical
assert objs_empty_list.equals(
objs_empty_dict
), "Empty list and empty dict should return same results"


def test_all_objects_filter_tags_copy_behavior():
"""Test that filter_tags dict is copied and not modified in place."""
original_filter = {"A": "1"}
original_copy = original_filter.copy()

# Call all_objects with the filter
all_objects(
package_name="skbase",
filter_tags=original_filter,
)

# Original dict should be unchanged
assert (
original_filter == original_copy
), "Original filter_tags dict should not be modified"


def test_all_object_tag_filter_regex():
"""Test all_objects filters by tag as expected, when using regex."""
import re
Expand Down