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] write docstring for RNN estimators #4558

Open
fkiraly opened this issue May 6, 2023 · 15 comments
Open

[DOC] write docstring for RNN estimators #4558

fkiraly opened this issue May 6, 2023 · 15 comments
Assignees
Labels
documentation Documentation & tutorials good first issue Good for newcomers module:classification classification module: time series classification module:regression regression module: time series regression

Comments

@fkiraly
Copy link
Collaborator

fkiraly commented May 6, 2023

The docstrings for estimators SimpleRNNRegressor and SimpleRNNClassifier are practically empty, these should be added.

At least a description of parameters and their defaults, that is relatively formulaic (from looking at the __init__)

Update: #4572 added parameters and defaults; a scientific summary is still missing (e.g., paper or description of the neural network)

@fkiraly fkiraly added documentation Documentation & tutorials module:classification classification module: time series classification module:regression regression module: time series regression good first issue Good for newcomers labels May 6, 2023
@wasup-yash
Copy link
Contributor

hey, I like to work on this issue and as I am new to the project could you provide the references to solve the issue?

@fkiraly
Copy link
Collaborator Author

fkiraly commented May 6, 2023

sure!

have a look at other estimators, e.g., KNeighborsTimeSeriesClassifier, and write the docstring in the same format.

If you are wondering about how to contribute, here is the contribution guide:
https://www.sktime.net/en/latest/get_involved/contributing.html

@wasup-yash
Copy link
Contributor

Thanks for the instructions @fkiraly

@fkiraly
Copy link
Collaborator Author

fkiraly commented May 7, 2023

welcome to sktime, @wasup-yash!

@achieveordie
Copy link
Collaborator

Yikes, must have missed adding that. @wasup-yash thanks for taking it up, if you have any questions regarding it please let us know.

@wasup-yash
Copy link
Contributor

wasup-yash commented May 7, 2023

@achieveordie yeah for sure, can you please point out where exactly in the path it needs to be updated?

@achieveordie
Copy link
Collaborator

Take a look at CNNClassifier for comparison. It mentions all the parameters that are passed to the constructor. If you look at SimpleRNNClassifier, it only has a reference and provides no information about the parameters. This is also true for SimpleRNNRegressor.

We use Read The Docs and Sphinx to auto-generate the documentation using the docstrings.

Also, before you start working on it, please ensure that you've got the development setup right- this will ensure the linting/formatting checks won't fail when you open a PR. Here's the developer's guide to assist you with it.

@wasup-yash
Copy link
Contributor

wasup-yash commented May 8, 2023

hey @achieveordie the parameters are the same in the SimpleRNNClassifier and SimpleRNNRegressor right ? and we need to only add parameters into it?

@achieveordie
Copy link
Collaborator

That's correct, both of these estimators have the same parameters and we intend to at least have 3 things, a one-line summary, parameter description and defaults and references at the end. Only parameter descriptions and defaults seem to be missing.

@wasup-yash
Copy link
Contributor

can you assign this to me so I start working on the PR?

@achieveordie
Copy link
Collaborator

There you go @wasup-yash, thanks :)

@wasup-yash
Copy link
Contributor

thanks, @achieveordie , I'll reach out incase of some issue :)

@wasup-yash
Copy link
Contributor

hey @achieveordie do SimpleRNNClassifier and SimpleRNNRegressor have any like references to add ?like in CNNClassifier we have Notes ----- Adapted from the implementation from Fawaz et. al https://github.com/hfawaz/dl-4-tsc/blob/master/classifiers/cnn.py

@achieveordie
Copy link
Collaborator

We're already adding the references, aren't we? Line 24

@wasup-yash
Copy link
Contributor

We're already adding the references, aren't we? Line 24

yeah but in CNNClassifier there was an extra ref as a note so that's why I had a doubt, thanks for helping

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

No branches or pull requests

3 participants