Skip to content

Add object_type param to named object check#136

Merged
RNKuhns merged 5 commits into
sktime:mainfrom
RNKuhns:update_named_object_check
Feb 24, 2023
Merged

Add object_type param to named object check#136
RNKuhns merged 5 commits into
sktime:mainfrom
RNKuhns:update_named_object_check

Conversation

@RNKuhns
Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns commented Feb 23, 2023

Reference Issues/PRs

Fixes #135

What does this implement/fix? Explain your changes.

Allows users to specify a specific class type to be used in the named object checks (is_sequence_named_objects and check_sequence_named_objects.

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

No

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #136 (9ab5d1a) into main (bd6acb6) will increase coverage by 0.08%.
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     #136      +/-   ##
==========================================
+ Coverage   82.35%   82.43%   +0.08%     
==========================================
  Files          31       31              
  Lines        2278     2289      +11     
==========================================
+ Hits         1876     1887      +11     
  Misses        402      402              
Impacted Files Coverage Δ
skbase/validate/_named_objects.py 100.00% <100.00%> (ø)
...base/validate/tests/test_iterable_named_objects.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 requested a review from fkiraly February 23, 2023 01:53
@RNKuhns RNKuhns added this to the Release 0.4.0 milestone Feb 23, 2023
@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 23, 2023

@fkiraly this should be good to go besides the fix in #137 that solves the pre-commit.ci failure.

Comment thread skbase/validate/_named_objects.py Outdated
depends on whether `seq_to_check` follows sequence of named
BaseObject format.

object_type : class, default=None
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.

I would say that it can also be a tuple of class, this might be useful, e.g., in pipelines

@@ -157,6 +174,7 @@ def check_sequence_named_objects(
seq_to_check: Union[Sequence[Tuple[str, BaseObject]], Dict[str, BaseObject]],
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.

I really don't like this additional amount of non-functional code generated from overload, but it's a matter of taste and not a blocker for me...

Comment thread skbase/validate/_named_objects.py Outdated
- If False, then whether or not the function returns True or False
depends on whether `seq_to_check` follows sequence of named BaseObject format.

object_type : class, default=None
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.

same 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.

Happy to merge.

I would suggest to extend the contract to allow tuples of classes, which it already does anyway (but doesn't say so in the docstring, and doesn't check in the tests).
Why: pipelines can be heterogeneous, and we may like to cover that case.

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 23, 2023

Good point on tuples. Will update docstring and tests pre-merge.

@RNKuhns RNKuhns merged commit e1b6a91 into sktime:main Feb 24, 2023
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] Add ability to specify specific class types to named object checks

3 participants