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] docstring for SimpleRNNClassifier #4572

Merged
merged 5 commits into from May 23, 2023
Merged

Conversation

wasup-yash
Copy link
Contributor

@wasup-yash wasup-yash commented May 11, 2023

Reference Issues/PRs

Towards #4558

What does this implement/fix? Explain your changes.

Added DocStrings for the [SimpleRNNClassifier](https://github.com/sktime/sktime/blob/main/sktime/classification/deep_learning/rnn.py#L20)

signed-off-by: yash sharma <trooper0018080@gmail.com>
Signed-off-by: Yash sharma <trooper0018080@gmail.com>
@wasup-yash wasup-yash requested a review from fkiraly as a code owner May 11, 2023 12:59
@fkiraly fkiraly changed the title Devdoc [DOC] docstring for SimpleRNNClassifier May 11, 2023
@fkiraly fkiraly added documentation Documentation & tutorials module:classification classification module: time series classification labels May 11, 2023
@yarnabrina
Copy link
Collaborator

@fkiraly just a quick question: why do most of deep_learning directory codes have a different docustring style from rest? These extra spaces before : are not really common in other sktime modules or even in numpydoc etc.

Not that it's objectively bad or anything, just asking because of inconsistent (if it matters for docstrings) style from most of other modules.

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2023

@fkiraly just a quick question: why do most of deep_learning directory codes have a different docustring style from rest? These extra spaces before : are not really common in other sktime modules or even in numpydoc etc.

Because it has been without maintenance for a period of time - most deep learning estimators lived originally in sktime-dl, which got abandoned as its maintainers did not keep up with the professionalisation of the software architecture in the main package (which also included better docstrings as a side effect).

As content has been migrated to sktime, the interfaces have been updated, but the docstrings often still are in the old style.

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 looks great!

Could you kindly remove the "should inherited fields" question from the docstring? Sth like this is better asked in the PR thread.

@wasup-yash
Copy link
Contributor Author

Thanks, this looks great!

Could you kindly remove the "should inherited fields" question from the docstring? Sth like this is better asked in the PR thread.

ok I am gonna remove it, thanks for the review

Signed-off-by: Yash Sharma <trooper0018080@gmail.com>
Signed-off-by: Yash Sharma <trooper0018080@gmail.com>
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.

now waitasec, I just noticed - and apologies for not noticing earlier - the defaults in the docstring don't actually agree with those in the __init__!

Can you kindly fix that? Also, where the default is None (e.g., ,optimizer), you should check what it is set to internally, e.g., is it Adam?

@wasup-yash
Copy link
Contributor Author

now waitasec, I just noticed - and apologies for not noticing earlier - the defaults in the docstring don't actually agree with those in the __init__!

Can you kindly fix that? Also, where the default is None (e.g., ,optimizer), you should check what it is set to internally, e.g., is it Adam?

oh, sorry for not noticing it from my side too, I'll work on it and update it for the review.

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.

Fixed outstanding items for the release

@fkiraly fkiraly merged commit a2715c3 into sktime:main May 23, 2023
23 checks passed
@wasup-yash
Copy link
Contributor Author

Fixed outstanding items for the release

thanks a lot @fkiraly for the fixes, I was indulged in uni exams and thought gonna work on it but you already fixed the docs, Really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials module:classification classification module: time series classification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants