Skip to content

[ENH] Improve skbase.utils#126

Merged
RNKuhns merged 10 commits into
sktime:mainfrom
RNKuhns:util_tests_docs
Feb 23, 2023
Merged

[ENH] Improve skbase.utils#126
RNKuhns merged 10 commits into
sktime:mainfrom
RNKuhns:util_tests_docs

Conversation

@RNKuhns
Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns commented Feb 5, 2023

Reference Issues/PRs

Completes the portion of #49 and #19 for the skbase.utils submodule. Note that I've included docstring examples for the functions even if they aren't public because I think it is helpful for developers to be able to get a quick glimpse of what the function does.

Also updates names of functions to be non-public (I'm guessing we don't want them in user API) and make minor tweaks to make functions more robust or generalizable (as discovered during test creation).

What does this implement/fix? Explain your changes.

The changes reflect:

  • Adding basic tests of skbase.utils (functions are tested further in tests of their downstream uses)
  • Improved docstrings of skbase.utils functionality (I believe these all now comply with numpydoc standard and have examples)
  • Renamed functions to be non-public and updated the names in import statements and calls elsewhere in the package
  • made minor improvements to functionality based on things I noticed when creating the tests/documenting the functionality that would easily make it more robust or improve its generality.

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

No new dependencies.

What should the reviewer focus their review on

I believe I added reasonable tests of the basic functionality of these utils (they are implicitly further tested in their downstream uses). So the key thing is whether you agree with the renaming to make the functionality non-public (i.e., not part of public API). Let me know if any of the functions in skbase.utils._iter.py or skbase.utils._nested_iter.py seem like something we want to include in the public API. Happy to switch the naming convention and add them to the API Reference.

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

@RNKuhns RNKuhns mentioned this pull request Feb 5, 2023
6 tasks
@RNKuhns RNKuhns requested a review from fkiraly February 5, 2023 20:41
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 5, 2023

Codecov Report

Merging #126 (020e200) into main (7bd9547) will increase coverage by 0.44%.
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     #126      +/-   ##
==========================================
+ Coverage   81.90%   82.35%   +0.44%     
==========================================
  Files          30       31       +1     
  Lines        2249     2278      +29     
==========================================
+ Hits         1842     1876      +34     
+ Misses        407      402       -5     
Impacted Files Coverage Δ
skbase/utils/_iter.py 100.00% <ø> (ø)
skbase/utils/__init__.py 100.00% <100.00%> (ø)
skbase/utils/_nested_iter.py 100.00% <100.00%> (+7.14%) ⬆️
skbase/utils/tests/test_iter.py 100.00% <100.00%> (ø)
skbase/utils/tests/test_nested_iter.py 100.00% <100.00%> (ø)
skbase/lookup/_lookup.py 95.48% <0.00%> (+0.34%) ⬆️
skbase/testing/utils/_dependencies.py 57.42% <0.00%> (+0.99%) ⬆️
skbase/validate/_named_objects.py 100.00% <0.00%> (+2.00%) ⬆️

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

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 9, 2023

@fkiraly do you want to take a look at this PR. Just cleans up some of the utility functionality.

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.

Apologies, I thought I had posted here already.

I am very happy with the improved docstrings, tests, etc.

However, I think at least some of the functions (e.g., flatten, unflatten) should remain public - in sktime, these are used in some concrete ensembles where they are useful to deal with index selection.

I do not have a too strong feeling about it though and would not block the PR.
However, leaving them private would mean that sktime has to copy-paste and maintain its own utilities around nested iterables, which seems wrong to me.

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 13, 2023

@fkiraly I am definitely up for leaving some functions part of the public API. It's an easy update on this PR.

Which do you think we should specifically make part of the public API?

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 18, 2023

@fkiraly let me know what you think in terms of the functions in utils you want public. I can switch the naming conventions and add them to the API really quick.

@RNKuhns RNKuhns mentioned this pull request Feb 18, 2023
@fkiraly
Copy link
Copy Markdown
Contributor

fkiraly commented Feb 22, 2023

Note that I've included docstring examples for the functions even if they aren't public because I think it is helpful for developers to be able to get a quick glimpse of what the function does.

I won't complain :-)

@fkiraly I am definitely up for leaving some functions part of the public API. It's an easy update on this PR.
Which do you think we should specifically make part of the public API?

flatten, unflatten. Maybe is_flat.

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.

Looks good (no strong opinion about flatten/unflatten, I would prefer them public)

Question, before we merge: there is an intersection with PR #130, the file _iter. Is this intended? If not, should be fixed; if so, is there a sequence this is intended to be merged?

@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 23, 2023

@fkiraly I'm down to make those three public. As you pointed out There is some interaction with #130. I'll merge that first and push the changes for this one and we can merge it tomorrow.

@RNKuhns RNKuhns added this to the Release 0.4.0 milestone Feb 23, 2023
@RNKuhns RNKuhns requested a review from fkiraly February 23, 2023 04:43
@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 23, 2023

@fkiraly this is failing because of fix needed for pre-commit.ci failure unrelated to the changes (that's in #137). Can you take a look at the changes and see if they meet expectations. If they look good, you can approve on condition of tests passing once #137 is merged and I'll merge things in order tomorrow.

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 with this - the public/private was not a blocker, I just wanted to flag the intersection. Given that you have a plan on how to handle the intersection/dependency here, all is good from my side.

@RNKuhns RNKuhns merged commit bd6acb6 into sktime:main Feb 23, 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.

3 participants