Skip to content

[ENH] tag manager mixin#138

Merged
RNKuhns merged 9 commits into
mainfrom
tagmanager
Mar 9, 2023
Merged

[ENH] tag manager mixin#138
RNKuhns merged 9 commits into
mainfrom
tagmanager

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented Mar 3, 2023

port of sktime/sktime#3630 to skbase

This PR factors out the tag management logic into a _FlagManager mixin, and generalizes the functionality so it can be used for multiple fields of tags.

Advantages:

  • increases cohesion and reduces coupling, separates concerns in the BaseObject class: state/parameter management and tag management
  • prepares the ground for managing configuration fields, e.g., for implementing a set_config similar to the new sklearn feature

Test coverage is already there for the base case, but should be added for a different tag field. Will do that with the PR that adds config handling.

(I called the flag manager since not all flags are tags, and interactions with the sklearn _get_tags)

A STEP is available here: sktime/enhancement-proposals#29

@fkiraly fkiraly added the implementing framework Implementing core skbase framework label Mar 3, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 3, 2023

Codecov Report

Merging #138 (db9846d) into main (fa53055) will increase coverage by 0.20%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   82.43%   82.64%   +0.20%     
==========================================
  Files          31       32       +1     
  Lines        2289     2316      +27     
==========================================
+ Hits         1887     1914      +27     
  Misses        402      402              
Impacted Files Coverage Δ
skbase/base/_base.py 81.81% <100.00%> (-1.40%) ⬇️
skbase/base/_tagmanager.py 100.00% <100.00%> (ø)

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 March 8, 2023 05:03
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.

@fkiraly see the requested docstring requests on #140 (which appears to have some logic repeated there -- likely since you needed some of this logic to add config interface).

@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented Mar 8, 2023

Addressed, see replies in #140

@fkiraly fkiraly requested a review from RNKuhns March 8, 2023 18:30
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.

LGTM now.

@RNKuhns RNKuhns merged commit 3cd2cbb into main Mar 9, 2023
@RNKuhns RNKuhns deleted the tagmanager branch March 9, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

implementing framework Implementing core skbase framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants