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] Added blank lines to properly render FourierFeatures docstring, sp_list (#5932) #5984

Merged
merged 1 commit into from Feb 28, 2024

Conversation

tiloye
Copy link
Contributor

@tiloye tiloye commented Feb 22, 2024

Reference Issues/PRs

Fixes #5932

What does this implement/fix? Explain your changes.

I added blank lines and line blocks to properly create the sp_list description, as required by the reStructuredText syntax.

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

No

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

No

Any other comments?

No

PR checklist

For all contributions
  • [ x] The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

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!

Btw, some docstrings use single-apostrophe (grave) by error instead of double-apostrope (double-grave) to render code snippets. If you encounter these, it would also be great to replace them.

The confusion is due to markdown havin single-grave for code snippets, but rst has double-grave.

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.

FYI, this is failing since the changes do not adhere to code formatting, specifically, "no trailing whitespaces".

See here how to set up local code formatting checks:
https://www.sktime.net/en/stable/developer_guide/coding_standards.html

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.

@tiloye, your PR includes changes to 17 files, I think this is because your branch is outdated?

Could you kindly update to most recent main to branch off?

@tiloye
Copy link
Contributor Author

tiloye commented Feb 24, 2024

@tiloye, your PR includes changes to 17 files, I think this is because your branch is outdated?

Could you kindly update to the most recent main to branch off?

I had an issue with git saying my local branch had diverged from the remote branch when i tried to push a change. I thought updating the branch and using git pull (with --rebase) was going to fix the issue. It fixed the issue, but it resulted in these 17 changes. I don't have a good knowledge of git, so I currently don't know what you mean by update to the most recent main to branch off.

I'm thinking of deleting the branch and restarting the process. Hope that won't be a problem?

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 24, 2024

I'm thinking of deleting the branch and restarting the process. Hope that won't be a problem?

That should work.

FYI, rebase often leads to issues like this (FYI @benHeid, @astrogilda due to recent discussions on rebase vs merge), I recommend merge from main.

@fkiraly fkiraly added the documentation Documentation & tutorials label Feb 25, 2024
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.

Excellent, thanks!

@fkiraly fkiraly changed the title [DOC] Added blank lines to properly render FourierFeatures sp_list (#5932) [DOC] Added blank lines to properly render FourierFeatures docstring, sp_list (#5932) Feb 25, 2024
Copy link
Collaborator

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

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.

No, it's not - I mistakenly looked at the wrong bullet point list (the one at the top).

Also, I think t/sp is better to read than the fraction?

@tiloye
Copy link
Contributor Author

tiloye commented Feb 25, 2024

https://sktime--5984.org.readthedocs.build/en/5984/api_reference/auto_generated/sktime.transformations.series.fourier.FourierFeatures.html#sktime.transformations.series.fourier.FourierFeatures

@tiloye @fkiraly can you please take a look here? I'm not sure if sp_list is being displayed correctly here as intended.

I'm still working on it. The second and third bullet points didn't start on a new line.

@tiloye
Copy link
Contributor Author

tiloye commented Feb 25, 2024

No, it's not - I mistakenly looked at the wrong bullet point list (the one at the top).

Also, I think t/sp is better to read than the fraction?

The fraction looks smaller. I'll change back to t/sp.

@tiloye
Copy link
Contributor Author

tiloye commented Feb 25, 2024

The documentation seems to be properly rendered now. However, I am not sure if the spaces between the bullet points (in the docstring) still adhere to the recommended docstring formatting guidelines.

I amended my previous commit and force pushed to the remote branch. Hope that won't be a problem during merge?

Copy link
Collaborator

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

Hi, I made some suggestions based on how it looks for me.

image

These are untested guesses from me, so they may or may not work. You can try to run locally and see how these appear and then commit and check on read the docs.

sktime/transformations/series/fourier.py Outdated Show resolved Hide resolved
sktime/transformations/series/fourier.py Outdated Show resolved Hide resolved
Copy link
Collaborator

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

No more observations from my side, approved assuming CI will pass successfully.

@fkiraly fkiraly merged commit 9f06b33 into sktime:main Feb 28, 2024
54 checks passed
@fkiraly fkiraly added the module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing label Feb 28, 2024
@tiloye tiloye deleted the fourier-doc branch February 29, 2024 14:41
fkiraly pushed a commit that referenced this pull request Mar 6, 2024
The invalid use of single-grave in docstrings has been fixed to
correctly render code snippets in API Reference Documentation

#### Reference Issues/PRs
This PR relates to these comments

#5932 (comment)
> code snippets do not render correctly (use single apostrophe instead
of double)

#5984 (review)
> Btw, some docstrings use single-apostrophe (grave) by error instead of
double-apostrope (double-grave) to render code snippets. If you
encounter these, it would also be great to replace them.
> 
> The confusion is due to markdown havin single-grave for code snippets,
but rst has double-grave.

#### What does this implement/fix? Explain your changes.

- In this PR, I have fixed the invalid use of single-grave at all places
in the code.
- This step is necessary as it enables the code snippets to render
properly in the API Reference Documentation.
- Although there are some valid uses of single-grave in the docstring
(math expressions) that I have not changed.
- Here is the [Script on Google
Colab](https://colab.research.google.com/drive/1McgC3cI-O40XYuQ0mUuRScVFcWt8puMp?usp=sharing)
that I used to auto-fix the docstrings

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

No

#### What should a reviewer concentrate their feedback on?

- There are minor changes (just addition of single-grave) but at many
lines in many files
- Some lines exceeded the character link so I split them into 2
following the PEP 8 guidelines

#### Did you add any tests for the change?

No, Although I am planning to add one with some guidance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] FourierFeatures sp_list is not rendering the bullet points correctly
3 participants