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] added usage examples to multiple estimator docstrings #6187

Merged
merged 17 commits into from Apr 26, 2024

Conversation

MihirsinhChauhan
Copy link
Contributor

Example added in Docstring of Estimators belongs classification task

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 22, 2024

Thanks, that's a lot of examples!

Code formatting is failing, probably an oversight. Just in case, here's the guide on setting it up locally:
https://www.sktime.net/en/stable/developer_guide/coding_standards.html

@MihirsinhChauhan
Copy link
Contributor Author

Updated Example according to standards (Code quality checked with pre-commit) , please give a review

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 22, 2024

still failing - there's one line that goes over the character limit (see logs)

@fkiraly fkiraly added the documentation Documentation & tutorials label Mar 22, 2024
@fkiraly fkiraly changed the title Example added in Docstring #4264 [DOC] added usage examples to multiples estimator docstrings Mar 22, 2024
@fkiraly fkiraly changed the title [DOC] added usage examples to multiples estimator docstrings [DOC] added usage examples to multiple estimator docstrings Mar 22, 2024
@MihirsinhChauhan
Copy link
Contributor Author

I had overlooked on one example,
Maybe this time there will be no problem.

Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I think you missed few section headers in docstrings, which leads to build failures in Read the Docs. Can you please fix these?

@MihirsinhChauhan
Copy link
Contributor Author

Example header added

Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the example headers.

  1. Please revert argument type change for estimator.
  2. Please use consistent example style for all examples, e.g. indentation, line length, etc. You can explore blacken-docs locally if it seems useful to you.


Examples
--------
>>> from sktime.classification.deep_learning.inceptiontime import InceptionTimeClassifier # doctest: +SKIP # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this noqa for? Is it for line length? Please fix by splitting into multiple lines instead of adding noqa.

Also, I think doctest need not be skipped in this line, but it's okay to have as well.

Comment on lines 118 to 123
>>> clf = ShapeDTW(n_neighbors=1,
... subsequence_length=30,
... shape_descriptor_function="raw",
... shape_descriptor_functions=None,
... metric_params=None
... ) # doctest: +SKIP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be good to add indentation after ... , otherwise it looks bad in the hosted docs (and inconsistent with other examples).

@@ -188,7 +203,7 @@ class labels (multi-output problem).

def __init__(
self,
estimator=None,
estimator,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is acceptable in a example addition PR. Plus it's potentially a breaking change as it now wants makes estimator a positional argument from a keyword argument. In fact this is what causes the test failure in CI

FYI @fkiraly

@MihirsinhChauhan
Copy link
Contributor Author

requested changes done

@MihirsinhChauhan
Copy link
Contributor Author

I'm not able to understand why tests are failing?

sktime/classification/distance_based/_shape_dtw.py Outdated Show resolved Hide resolved
sktime/classification/feature_based/_fresh_prince.py Outdated Show resolved Hide resolved
sktime/classification/ensemble/_ctsf.py Outdated Show resolved Hide resolved
sktime/classification/feature_based/_tsfresh_classifier.py Outdated Show resolved Hide resolved
sktime/classification/feature_based/_tsfresh_classifier.py Outdated Show resolved Hide resolved
@MihirsinhChauhan
Copy link
Contributor Author

changes done

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 30, 2024

You need to add a doctest skip to those estimators that rely on soft dependencies, so the doctest is not executed, see
https://www.sktime.net/en/latest/developer_guide/dependencies.html#handling-soft-dependencies

yarnabrina pushed a commit that referenced this pull request Apr 8, 2024
Adding usage example to estimator  
In PR #6187 I had added multiple usage example, but due to some reason
I'm not able to understand where test failing. so on this Pr I will
added usage example one by one
@fkiraly fkiraly merged commit 15cd1dd into sktime:main Apr 26, 2024
12 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants