Add tests for BaseObject and BaseEstimator

[RNKuhns]

Reference Issues/PRs

Fixes #61 and fixes #63.

What does this implement/fix? Explain your changes.

This adds additional tests of the BaseEstimator interface as described in #61 and additional tests of the BaseObject interface as described in #63.

Does your contribution introduce a new dependency? If yes, which one?

No new dependencies.

What should a reviewer concentrate their feedback on?

Does this provide desired testing of interface.

[codecov-commenter]

Codecov Report

Merging #62 (263f53e) into main (c3f5443) will increase coverage by 3.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   54.80%   57.85%   +3.04%     
==========================================
  Files          16       16              
  Lines        1352     1350       -2     
==========================================
+ Hits          741      781      +40     
+ Misses        611      569      -42     

Impacted Files	Coverage Δ
skbase/_base.py	81.30% <100.00%> (+17.94%)	⬆️

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

[RNKuhns]

@fkiraly I'm making good progress towards adding more tests of our base functionality.

Just plan to add tests for the following yet as part of this PR:

• BaseObject.get_test_params()
• BaseObejct.create_test_instance():
• BaseObject.create_test_instances_and_names()
• BaseObject._has_implementation_of()

[RNKuhns]

@fkiraly this should be ready for you to review. I been able to cover most of our base functionality outside of the TagAliaserMixin.

[fkiraly]

Looked in sktime whether what is there right now is all here, and found some diffs:

• sktime also has a test `test_components for the components inspection interface
• test_base_sktime has some compatibility tests with sklearn

[fkiraly]

Most of these make sense and are indeed useful additions.

My main comment (blocking until discussed, not necessarily actioned) is, many of these tests intersect with what TestAllObjects is doing, for concrete classes.

To avoid duplication, should we not instead run check_object with a test class?

As a test for BaseObject, using mock objects, rather than as a test for the object subjected to check_object - or is that too many inversions for your taste?

Another issue, sktime has some more functionality around fitted parameter inspection, although we can port that later.

[RNKuhns]

Another issue, sktime has some more functionality around fitted parameter inspection, although we can port that later.

I saw that, I think we want to have a PR that ports the additional sktime functionality as part of this release (e.g. we sync things back up), but we do that separate from this PR.

[RNKuhns]

Most of these make sense and are indeed useful additions.

My main comment (blocking until discussed, not necessarily actioned) is, many of these tests intersect with what TestAllObjects is doing, for concrete classes.

To avoid duplication, should we not instead run check_object with a test class?

As a test for BaseObject, using mock objects, rather than as a test for the object subjected to check_object - or is that too many inversions for your taste?

I'm generally a fan of having separate tests for the base class and the concrete classes, but we can talk about it.

[RNKuhns]

Looked in sktime whether what is there right now is all here, and found some diffs:

• sktime also has a test `test_components for the components inspection interface
• test_base_sktime has some compatibility tests with sklearn

There is a test_components in skbase, but it was already there (not being added in this PR). The tests in test_base_sktime focus on the fitted parameter interface. We'll need to add those in the separate PR to add in the new functionality from sktime that has been added since we ported the original BaseObject into skbase.

[fkiraly]

Agreed with resolutions and otherwise explanations where it turned out no action is needed in this PR.

There are some items that arise to do in other PR, mostly to do with updating current BaseObject with changes made since then in sktime, we should keep track of those: _required_parameters removed; functionality around fitting and fitted params.