-
Notifications
You must be signed in to change notification settings - Fork 636
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
base: main
Are you sure you want to change the base?
Conversation
@fkiraly @yarnabrina reviews appreciated if possible :) |
There was a problem hiding this 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.
|
||
|
||
Alternatively, to install the package via ``conda``: | ||
.. code-block:: bash |
There was a problem hiding this comment.
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.
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 |
is this ready for review, @julian-fong? |
This is ready for review. |
@fkiraly / @julian-fong do we have the read the docs build per pull request now? I'd like to see whether it renders correctly. |
@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. |
There was a problem hiding this 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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 |
@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. |
@yarnabrina, @julian-fong - may I kindly ask, what is the status of this? |
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! |
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
[DOC] review of developer documentation #1635 and [DOC] Request for Recommended Environment Setup for Temporal Fusion Transformer Tutorial #1698
pre-commit install
.To run hooks independent of commit, execute
pre-commit run --all-files