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] Fixed Issue #3826 - incomplete ForecastingPipeline docstring #3840

Merged
merged 10 commits into from
Jan 11, 2023

Conversation

darshitsharma
Copy link
Contributor

@darshitsharma darshitsharma commented Nov 27, 2022

Fixes #3826 - the docstring of ForecastingPipeline was incomplete.
This PR adds a complete docstring.

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 helpful!

What was meant in the issue was the description of the algorithm, what happens in fit etc.

Kindly have a look at TransformedTargetForecaster please, that is how it should look like.

@darshitsharma
Copy link
Contributor Author

Hello @fkiraly ,
I applied changes the way you mentioned. I am new to open-source and would like to have your guidance here.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 11, 2022

Thanks for your work!

I would recommend, have a look at TransformedTargetForecaster please, that is how it should look like.

In more detail, it would be great if you could describe how the steps are applied in sequence, for fit and predict.

@darshitsharma
Copy link
Contributor Author

In ForecastingPipeline, described the working of predict and fit in the way they are described in TransformedTargetForecaster.

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!

Can you kindly format the docstrings so that you do not exceed line length of 80 symbols?
Also, kindly don't indent too much, or it will look strange when it renders.

@darshitsharma
Copy link
Contributor Author

Hi,
I checked that the line wrap was 81 characters instead of 80 characters, corrected my mistake, and removed unnecessary indentation.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 24, 2022

Thanks for your contribution!

To get this merged, kindly make sure your linting runs, guideline here:
https://www.sktime.org/en/stable/developer_guide/coding_standards.html

@darshitsharma
Copy link
Contributor Author

darshitsharma commented Dec 31, 2022

Hey @fkiraly,
I am using Visual Studio Code and I have installed all dependencies for sktime to use pre-commit.
The linting error is with flake8 max-line-length=88. Can you suggest something to resolve this?
flake8

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 31, 2022

Can you suggest something to resolve this?

The general approach would be to look up the error code in flake8 - in your case E501 and check what you need to do.

In this case, it says for of your lines are too long. The file and line number is given in the error message.

The action you have to carry out is to find those lines and ensure they are not longer than 88 characters, e.g., by introducing a sufficient amount of newlines.

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.

Nice, thanks a lot!

I left some comments in the docstring, some details need to be corrected or sharpened.

@fkiraly fkiraly changed the title DOC - Fixed Issue #3826 - incomplete ForecastingPipeline docstring [DOC] Fixed Issue #3826 - incomplete ForecastingPipeline docstring Jan 10, 2023
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 10, 2023

@darshitsharma, thanks!
I made some small changes to make the docstring more similar to TransformedTargetForecaster, your contribution is much appreciated!

@darshitsharma
Copy link
Contributor Author

@fkiraly , Thanks! for being so helpful throughout. I learned a lot about documenting code here. I hope to learn more from you and contribute to sktime.

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.

(see above)

@fkiraly fkiraly merged commit 7805ac1 into sktime:main Jan 11, 2023
klam-data pushed a commit to CodeSmithDSMLProjects/sktime that referenced this pull request Jan 18, 2023
Fixes sktime#3826 - the docstring of `ForecastingPipeline` was incomplete.
This PR adds a complete docstring.
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.

[DOC] fix incomplete ForecastingPipeline docstring
2 participants