Skip to content

[ENH] ensure that all_objects always returns (class name/class) pairs#115

Merged
fkiraly merged 2 commits into
mainfrom
lookup-classname
Jan 24, 2023
Merged

[ENH] ensure that all_objects always returns (class name/class) pairs#115
fkiraly merged 2 commits into
mainfrom
lookup-classname

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented Jan 15, 2023

all_objects in vanilla form returns a list of (str, BaseObject) pairs.

Currently, these pairs do not need to satisfy the condition that for the pair (name, klass) it holds that name = klass.__name__.
An example of this case is when a class is assigned to a variable name other than the class name. In that case name is the variable name, not the class name. E.g., doing sth like this

class Test:
    def __init__(self, stuff):
        more_stuff

test2 = Test

results in the class Test being returned twice, once with name test2 and once with name Test.

This can be quite confusing and violate an interface contract that is not strictly stated as guaranteed, but may be assumed by users of the utility (and, apparently, by the status quo in sktime as well as the failing tests in the attempted refactor PR sktime/sktime#3777 e.g., see the line assert estimator[0] == estimator[1].__name__ in test_all_estimators_by_scitype)

I would argue that it should always hold that name = klass.__name__ for the returned pairs for that reason.

The alternative solution to this issue would be to make crystal clear in the docstring what is being guaranteed, and what not, about the name/estimator pairs returned.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 15, 2023

Codecov Report

Merging #115 (b3107ef) into main (471000d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage   77.93%   77.93%           
=======================================
  Files          23       23           
  Lines        1867     1867           
=======================================
  Hits         1455     1455           
  Misses        412      412           
Impacted Files Coverage Δ
skbase/lookup/_lookup.py 95.48% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RNKuhns RNKuhns self-requested a review January 23, 2023 00:38
Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. Hadn't considered the case where class was assigned to variable. LGTM.

@fkiraly fkiraly merged commit 5004893 into main Jan 24, 2023
fkiraly pushed a commit that referenced this pull request Oct 14, 2025
#### Reference Issues/PRs

Fixes #112 by adding a test


#### What does this implement/fix? Explain your changes.

This does not fix anything, since the bug is apparently already fixed by
#115. But it adds a minimal test.

The test creates a dummy module to test the behaviour of `all_objects`.
This dummy module might be useful for testing, because it is independent
from the existing code. I verified, that the test fails (in an expected
way) before #115 was merged (while all other tests passed). So this test
should give some additional safety in the future.
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.

3 participants