Skip to content

[BUG] fixes and improvements to all_objects#69

Merged
RNKuhns merged 7 commits into
mainfrom
all_objects-fixes
Nov 23, 2022
Merged

[BUG] fixes and improvements to all_objects#69
RNKuhns merged 7 commits into
mainfrom
all_objects-fixes

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented Nov 13, 2022

This fixes some bugs and makes improvements to all_objects that are in sktime.

Bugs:

Improvements:

  • docstrings for modules_to_ignore parameters
  • docstring of all_objects fixed to make reference to objects, not estimators

@fkiraly fkiraly added the bug Something isn't working label Nov 13, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 13, 2022

Codecov Report

Merging #69 (22ce6fc) into main (7135891) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main      #69   +/-   ##
=======================================
  Coverage   54.80%   54.80%           
=======================================
  Files          16       16           
  Lines        1352     1352           
=======================================
  Hits          741      741           
  Misses        611      611           
Impacted Files Coverage Δ
skbase/_lookup.py 42.21% <66.66%> (ø)

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

@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented Nov 13, 2022

maybe we should also add a test for this?

@fkiraly fkiraly marked this pull request as ready for review November 13, 2022 16:35
@fkiraly fkiraly requested a review from RNKuhns November 18, 2022 10:58
@RNKuhns
Copy link
Copy Markdown
Contributor

RNKuhns commented Nov 22, 2022

@fkiraly there is still a bit of formatting weirdness in the all_object docstring when it renders in the read-the-docs check.

Do you mind taking a look at that and updating the format? I'm going to be opening up a separate PR for the tests.

@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented Nov 22, 2022

Do you mind taking a look at that and updating the format? I'm going to be opening up a separate PR for the tests.

Just looked at it, looks ok to me?

I also did not change the general format or indentation, I think?

Do you mean the "note" boxes? I thought this was ok, but we can change the indentation.

@RNKuhns
Copy link
Copy Markdown
Contributor

RNKuhns commented Nov 22, 2022

@fkiraly I'm fine leaving the docstring as is for now. I thought I was seeing one part bolded that didn't seem like it should be (the boxes always through me for a loop, but I get its an indentation thing). I think we have an open issue related to the docstring for this, but I'll double-check when I add the tests for lookup functionality.

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. Will merge and handle tests and potential tweak to docstring in forthcoming PR.

@RNKuhns RNKuhns merged commit 5ae8246 into main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] all_objects argument object_types does not work

3 participants