Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOC] Update Docstring for ColumnConcatenator #4272

Merged
merged 4 commits into from Feb 28, 2023

Conversation

JonathanBechtel
Copy link
Contributor

Reference Issues/PRs

References PR #4262

What does this implement/fix? Explain your changes.

This PR adds an updated docstring that provides an example for how to use it, as well as a slightly more detailed explanation about its use case.

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

No

What should a reviewer concentrate their feedback on?

Making sure the new example is adequate to provide context about how to use the tool

Did you add any tests for the change?

Being that this is just an update to a DOC string, there is no need to add a unit test

Any other comments?

I think more of these PR's should be created because it often difficult to see where to import different tools from. It was too difficult to figure out how to use such a simple piece of the program.

PR checklist

For all contributions
  • [ X] I've added myself to the list of contributors.
  • [X ] The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

Copy link
Collaborator

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

Thanks!

One minor thing, I don't think we need to (or should) skip the doctest lines. This is just in a case where we have soft dependencies.

In doctest, the output also does not need to be preceded by >>> (or it will be considered an input). So, you may want to remove this in front of the array at the end.

Further, would using an example panel dataset, e.g., using _make_panel from sktime.utils._testing.panel (with n_columns=2) also be illustrative?

Regarding "making more of these" - absolutely! Much appreciated!
Here's the umbrella issue: #4264

@JonathanBechtel
Copy link
Contributor Author

All of those changes are fine, and I can update them, with one exception: from sktime.transformations.panel.compose import ColumnConcatenator takes up 92 lines so it fails the flake8 check. So I think that exception has to stay. Definitely think the panel dataset would be a good example as well, so will add it.

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 26, 2023

So I think that exception has to stay. Definitely think the panel dataset would be a good example as well, so will add it.

Sure - the noqa exception is fine, that doesn't skip it in the doctest.

An alternative, though slightly more ugly for the user (and I prefer your workaround, but just FYI) is having an import with brackets and newline e.g.,

from longmodulename.longmodulename.longmodulename import (
    this_is_what_we_import
)

@JonathanBechtel
Copy link
Contributor Author

Made the changes. In the future, is it okay to just reference the umbrella issue when a new PR is opened and skip making it a separate issue?

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 27, 2023

Thanks a lot!

In the future, is it okay to just reference the umbrella issue when a new PR is opened and skip making it a separate issue?

Of course.

Copy link
Collaborator

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

Thanks!

This is failing due to conventions of doctests.

In doctest:

  • >>> is start of an expression
  • ... is used when the same expression goes over multiple rows. One >>> and all subsequent ... are interpreted as one block of code.
  • if you want "expeted output", it should start at the line without ....

That is, the ... in lines 228, 229 are correct - in lines 232-240 you probably don't want ...

@RNKuhns
Copy link
Contributor

RNKuhns commented Feb 27, 2023

@JonathanBechtel you can also add comments in between examples (have to have blank lines around those lines).

To make it easy check out the example section in numpydoc’s docs and examples in skbase.

@JonathanBechtel
Copy link
Contributor Author

@JonathanBechtel you can also add comments in between examples (have to have blank lines around those lines).

To make it easy check out the example section in numpydoc’s docs and examples in skbase.

@RNKuhns To be clear, lines #96, #104, and #110 in [skbase] are examples of what you're describing, yes?

@JonathanBechtel
Copy link
Contributor Author

@fkiraly Any idea why some small changes to a docstring would cause compatibility failures for unix operating systems?

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 28, 2023

They shouldn't, in fact it is an instance of this known stochastic failure: #3850

Copy link
Collaborator

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

Great, thanks!

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.

None yet

3 participants