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

Add additional clarifying details to documentation guide (in developer's guide) #1315

Merged
merged 23 commits into from Sep 8, 2021

Conversation

RNKuhns
Copy link
Contributor

@RNKuhns RNKuhns commented Aug 16, 2021

Reference Issues/PRs

Relates to #1245

What does this implement/fix? Explain your changes.

Based on feedback during the docsprint, I've included clarifying content to the documentation portion of the developer guide.

Does your contribution introduce a new dependency? If yes, which one?

Are the added clarifying details clear enough that developers can easily follow them?
Are there other areas where clarification is needed?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

@RNKuhns RNKuhns requested a review from mloning as a code owner August 16, 2021 19:01
@RNKuhns
Copy link
Contributor Author

RNKuhns commented Aug 16, 2021

Ignore the boxcox.py change, that was meant to be separate (see #1314).

@GuzalBulatova, @SveaMeyer13, @Lovkush-A and @aiwalter can you look at the read the docs build from this PR and let me know if:

  • The new clarifications (e.g. how to do cross-references, etc) are clear
  • Whether you think there are other areas clarifications that would be useful

Copy link
Collaborator

@Lovkush-A Lovkush-A left a comment

Choose a reason for hiding this comment

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

I like the extra information included. Suggestions are:

  • typos
  • slight rewordings which i think are clearer or more concise
  • inclusion of good examples section

docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Outdated Show resolved Hide resolved
docs/source/developer_guide/documentation.rst Show resolved Hide resolved
@Lovkush-A
Copy link
Collaborator

Typos outside of changes in this PR (and so I cannot suggest directly in the code...)

Line 14. api_refernce
Line 22. The url does not become a hyperlink

RNKuhns and others added 7 commits August 16, 2021 16:40
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
Co-authored-by: Lovkush <lovkush@gmail.com>
@RNKuhns
Copy link
Contributor Author

RNKuhns commented Aug 16, 2021

Typos outside of changes in this PR (and so I cannot suggest directly in the code...)

Line 14. api_refernce
Line 22. The url does not become a hyperlink

Thanks for pointing those out. I'll push a commit to fix them.

@RNKuhns RNKuhns requested a review from Lovkush-A August 16, 2021 20:51
Copy link
Collaborator

@Lovkush-A Lovkush-A left a comment

Choose a reason for hiding this comment

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

looks good! (note that in contractbleBOSS, the joint reference [1,2] did not become a hyperlink)

@RNKuhns
Copy link
Contributor Author

RNKuhns commented Aug 17, 2021

@Lovkush-A thanks for taking a look and approving. Do you mind opening a PR to fix the link rendering in ContractableBoss?

@Lovkush-A
Copy link
Collaborator

Do you mind opening a PR to fix the link rendering in ContractableBoss?

I can do it in my PR in which I am updating three or four other docstrings. I think I read the CI/CD system is strugging and that having fewer PRs should help.

@RNKuhns
Copy link
Contributor Author

RNKuhns commented Aug 19, 2021

@mloning are you fine if we merge this since it only affects docs?

Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

Yes, this has been reviewed so happy for this to go in

@mloning mloning merged commit 3f09ac7 into sktime:main Sep 8, 2021
@mloning
Copy link
Contributor

mloning commented Sep 8, 2021

Now merged - thanks @RNKuhns!

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.

None yet

5 participants