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] General PR to improve docs #1705

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

julian-fong
Copy link

@julian-fong julian-fong commented Oct 28, 2024

Description

This PR aims to improve current docstring and revamp the contribute/installation page for users looking to get working copies of pytorch-forecasting or would like to get a developmental version for contributing.

Checklist

@julian-fong
Copy link
Author

@fkiraly @yarnabrina reviews appreciated if possible :)

Copy link
Member

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

This is an incomplete review. I think the last part of changes are from the deleted file, any reason to not have it as it is? In a lot of Github repositories, CONTRIBUTING.md or similar seems quite standard.

docs/source/getting-started.rst Outdated Show resolved Hide resolved
docs/source/installation.rst Outdated Show resolved Hide resolved
docs/source/installation.rst Outdated Show resolved Hide resolved
docs/source/installation.rst Outdated Show resolved Hide resolved


Alternatively, to install the package via ``conda``:
.. code-block:: bash
Copy link
Member

Choose a reason for hiding this comment

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

I think it needs a new line before this. Please verify.

docs/source/installation.rst Outdated Show resolved Hide resolved
@julian-fong
Copy link
Author

This is an incomplete review. I think the last part of changes are from the deleted file, any reason to not have it as it is? In a lot of Github repositories, CONTRIBUTING.md or similar seems quite standard.

I've opted to replace the contributing.md with installation.md and include the contributing section inside the installation.md file. This was also done to match sktime and skpro, do you have a preference otherwise? I suppose they could be split into two separate files, I'm good with either way

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 6, 2024

is this ready for review, @julian-fong?

@julian-fong
Copy link
Author

is this ready for review, @julian-fong?

This is ready for review.

@yarnabrina
Copy link
Member

@fkiraly / @julian-fong do we have the read the docs build per pull request now? I'd like to see whether it renders correctly.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 9, 2024

@yarnabrina, no, we do not have this yet.

I opened an issue to track this as a maintenance/CI work item: #1708

Do you know how to enable this? If yes, kindly add instructions or links in #1708.

Copy link
Member

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

Added few comments. Since RTD is enabled now, next commit should help us to see the docs easily.

Consider using free distributions and channels for package management,
and be aware of applicable terms and conditions.

In the ``conda`` terminal:
Copy link
Member

Choose a reason for hiding this comment

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

This line looks odd. Did you miss a command, or did you intend it to be part if point 2?

Copy link
Author

Choose a reason for hiding this comment

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

I think everything is intended here, I copied it off of the sktime installation instructions - see https://www.sktime.net/en/stable/installation.html#full-developer-setup-for-contributors-and-extension-developers

Copy link
Member

Choose a reason for hiding this comment

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

To be precise what looked odd to me, that is this section. It seems bullets 2-4 are under bullet 1, and bullet 5 is once again at same level as bullet 1.

I am not formally requesting a change, but this did look weird to me personally, both here and in sktime too (which I definitely missed earlier).

docs/source/installation.rst Outdated Show resolved Hide resolved
Comment on lines +51 to +55
To install the Pytorch Lightning library, please visit their `official page <https://lightning.ai/docs/pytorch/stable/starter/installation.html>`__ or run:

.. code-block:: bash

pip install lightning
Copy link
Member

Choose a reason for hiding this comment

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

Question: does is need to be a separate install?

Copy link
Author

Choose a reason for hiding this comment

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

It does not need to be a separate install since it is one of the pytorch-forecasting core dependencies. I thought it would be a good idea to include a step to install the package since it is such an important library (similar to torch).

We can remove it if its not needed


.. code:: bash

git clone [email protected]:<username>/sktime/pytorch-forecasting.git
Copy link
Member

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 the path for personal forks is the most common path. Won't it be <username>/pytorch-forecasting instead of <username>/sktime/pytorch-forecasting by default?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not 100% sure to be honest @fkiraly do you know if this is the correct path?

@julian-fong
Copy link
Author

I suppose it would be a good idea to have a couple users try and follow the instructions to see if everything works properly or not

@julian-fong
Copy link
Author

@yarnabrina how does it look? I see that on the page there is an empty 'section navigation' on the left side - not exactly sure how to remove that

@yarnabrina
Copy link
Member

@yarnabrina how does it look? I see that on the page there is an empty 'section navigation' on the left side - not exactly sure how to remove that

My guess is in the rst itself, not sure. I think specifying the initial ".. " as we have in sktime may have something to do with it, but it's a blind guess and not tested.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 19, 2024

@yarnabrina, @julian-fong - may I kindly ask, what is the status of this?
Still worked on? Ready to merge/review?

@fkiraly fkiraly added the documentation Improvements or additions to documentation label Nov 19, 2024
@julian-fong
Copy link
Author

@yarnabrina, @julian-fong - may I kindly ask, what is the status of this?

Still worked on? Ready to merge/review?

It is ready for review. @yarnabrina already had a review and approved it also, confirmation of that is on the discord pytorch forecasting channel. You can also give a review if you would like!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants