Skip to content

[ENH] Update meta classes and add unit tests#107

Merged
fkiraly merged 24 commits into
sktime:mainfrom
RNKuhns:meta_refactor
Apr 18, 2023
Merged

[ENH] Update meta classes and add unit tests#107
fkiraly merged 24 commits into
sktime:mainfrom
RNKuhns:meta_refactor

Conversation

@RNKuhns
Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns commented Jan 13, 2023

Reference Issues/PRs

Fixes #106.

What does this implement/fix? Explain your changes.

This PR will add unit tests of the meta class functionality. It will also provide an update of the provided functionality to make it generally consistent with skbase design principles.

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

No new dependencies.

What should a reviewer concentrate their feedback on?

Updates to BaseMetaObject (renamed from BaseMetaEstimator) and whether tests cover reasonable use-cases.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 13, 2023

Codecov Report

Attention: Patch coverage is 54.31034% with 106 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (39f6adf) to head (185aa5a).
Report is 209 commits behind head on main.

Files Patch % Lines
skbase/base/_meta.py 21.48% 106 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   82.68%   84.09%   +1.40%     
==========================================
  Files          32       34       +2     
  Lines        2322     2383      +61     
==========================================
+ Hits         1920     2004      +84     
+ Misses        402      379      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Ah yes, I forgot to port this. Good spot.

@fkiraly
Copy link
Copy Markdown
Contributor

fkiraly commented Jan 13, 2023

maybe one thing for you to review, whether you like (or not) the implied inheritance order.

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Jan 14, 2023

maybe one thing for you to review, whether you like (or not) the implied inheritance order.

Do you mean in downsteram usage?

class SomePipeline(BaseMetaObject, BaseObject):
    ...

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Jan 14, 2023

@fkiraly I'm going to leave this open for a bit and add unit tests of this functionality. For ease, can you point me to related sktime unit tests. I didn't see any directly testing this where the sktime.base tests were located, but there is test coverage. Is this handled in tests of classes using _HeterogeneousMetaEstimator?

@fkiraly
Copy link
Copy Markdown
Contributor

fkiraly commented Jan 15, 2023

Do you mean in downsteram usage?

Yes.

One issue that for me has recently come up is that the meta-estimator can be a BaseObject and not BaseEstimator, or it can be both.

In either case, it makes sense to implement default functionality for the parameter getters get_params, get_fitted_params, but the second one is only "present" for BaseEstimator descendants.

I've been moving back and forth in this PR as it was one of the outstanding base framework issues in sktime to give the pipelines a proper get_fitted_params, and it leads to loading both BaseObject and BaseEstimator specific defaults into the meta-class. sktime/sktime#4110

Not sure whether there is a better way, at least under this downstream usage.

@fkiraly I'm going to leave this open for a bit and add unit tests of this functionality. For ease, can you point me to related sktime unit tests. I didn't see any directly testing this where the sktime.base tests were located, but there is test coverage. Is this handled in tests of classes using _HeterogeneousMetaEstimator?

Yes, correct - most of the coverage comes from integration tests which require the functionality in the mixin to work correctly in order to pass.

There are further punctual tests when it comes to the specific tag behaviour of things like pipelines, or logic of the meta-class child.

I also do not think there are any direct tests, but I might be mistaken.

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 5, 2023

Do you mean in downsteram usage?

Yes.

One issue that for me has recently come up is that the meta-estimator can be a BaseObject and not BaseEstimator, or it can be both.

In either case, it makes sense to implement default functionality for the parameter getters get_params, get_fitted_params, but the second one is only "present" for BaseEstimator descendants.

I'm thinking we might want want 2 mixins.

  1. BaseMetaObject mixin that has meta get_params and set_params (and other functionaly that applies to objects and estimators).
  2. BaseMetaEstimator mixin that inherits from BaseMetaObject (does alll the same stuff), and also has estimator specific things like retrieving fitted parameters.

From a user perspective, we'd document that the choice of the mixin to use is dependent on whether your building a meta object or meta estimator, which seems straight-forward enough. For example:

from skbase.base import BaseEstimator, BaseMetaestimator, BaseMetaObject, BaseObject

class SomeNeatMetaObject(BaseMetaObject, BaseObject):
    ...

class SomeNeatMetaEstimator(BaseMetaEstimator, BaseEstimator):
    ...

From a maintenance perspective, we'd have the same functionality and not double anything up.

I'll work up this and some integration tests today.

Comment thread skbase/tests/test_meta.py Fixed
Comment thread skbase/tests/test_meta.py Fixed
@fkiraly
Copy link
Copy Markdown
Contributor

fkiraly commented Feb 6, 2023

From a maintenance perspective, we'd have the same functionality and not double anything up.

I suppose that makes sense, assuming they are clearly presented as mixins.

Not more complex to the user than the alternatives.

@RNKuhns RNKuhns mentioned this pull request Feb 18, 2023
@RNKuhns RNKuhns added this to the Release 0.4.0 milestone Feb 23, 2023
Comment thread skbase/utils/_iter.py Fixed
Comment thread skbase/base/_meta.py Fixed
Comment thread skbase/utils/_iter.py Fixed
Comment thread skbase/utils/_iter.py Fixed
Comment thread skbase/base/_meta.py Fixed
Comment thread skbase/base/_meta.py Fixed
Comment thread skbase/utils/_iter.py

@overload
def make_strings_unique(str_list: Tuple[str, ...]) -> Tuple[str, ...]:
... # pragma: no cover

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
Comment thread skbase/utils/_iter.py

@overload
def make_strings_unique(str_list: List[str]) -> List[str]:
... # pragma: no cover

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
Comment thread skbase/base/_meta.py

@overload
def _check_names(self, names: List[str], make_unique: bool = True) -> List[str]:
... # pragma: no cover

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
Comment thread skbase/base/_meta.py
def _check_names(
self, names: Tuple[str, ...], make_unique: bool = True
) -> Tuple[str, ...]:
... # pragma: no cover

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
Comment thread skbase/tests/test_meta.py Fixed
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.

[ENH] Refactor skbase.base._meta and add unit tests

4 participants