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

Formatted docstrings in files to pass with pydocstyle's criteria. #72

Merged
merged 19 commits into from
Mar 28, 2025

Conversation

Averagenormaljoe
Copy link
Contributor

@Averagenormaljoe Averagenormaljoe commented Mar 25, 2025

Pull Request

Description

I changed and added docstrings in the '.py' files to match pydocstyle's criteria. In addition, docstrings with only single double quotes have been changed to triple quotes.

Relates to issues #1 and #66.

Fixes

How Has This Been Tested?

This was tested using the 'pydocstyle' command in the repository.

  • Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@jacobbieker
Copy link
Member

This looks good! Is there any chance you could also fix the last few pre-commit issues? I know they aren't strictly part of this issue, but then it could be a green pre-commit check!

@Averagenormaljoe
Copy link
Contributor Author

Averagenormaljoe commented Mar 26, 2025

I can, but one of them is related to implicit import in the 'init.py' for the layers module for 'from .layers import *", where changing it to explicit import may affect the importing packages when the user is importing from packages, and the second one would require reducing the length of the summary text in the docstring. Would changing the length of the summary text be fine?

@Averagenormaljoe
Copy link
Contributor Author

I will get started on the undefined name error caused by explicit imports in the “init.py” in the base of the directory.

 

@Averagenormaljoe
Copy link
Contributor Author

I have made changes to have the pre-commit work. @jacobbieker

@Averagenormaljoe
Copy link
Contributor Author

As this pull request solves the Ruff linter pre-commit issue, should it be linked to this pull request in the description of the issue alongside the pydocstyle one?

1 similar comment
@Averagenormaljoe
Copy link
Contributor Author

As this pull request solves the Ruff linter pre-commit issue, should it be linked to this pull request in the description of the issue alongside the pydocstyle one?

@jacobbieker
Copy link
Member

Yes, you can add that it fixes that issue in the PR description here, that would link it enough and close that issue as well when this is merged.

Copy link
Member

@jacobbieker jacobbieker 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 overall! Just a few changes ideally.

Initialize the Upsample Residual Convolution.

Args:
input_channels: int,
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above

center_crop_size:
forecast_steps:
**kwargs:
image_encoder: string
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, add the description for what these args are, like in the above ones. But there are a lot here, so feel free to leave this for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I will work on adding descriptions to the args after I fix the Ruff linter issue.

@Averagenormaljoe
Copy link
Contributor Author

Thank you, I have added the suggested changes to the commit. Currently, Ruff linter has an issue with the docstring in the 'metnet2.py' file summary description being 1 character longer than 100. We could remove the period from the sentence to solve the Ruff linter error, but then the Pydocstyle linter would error that the sentence does not contain a period at the end of it.

…Net2' in the the 'models/metnet2.py' file to match the criteria of Ruff
@Averagenormaljoe
Copy link
Contributor Author

In the summary text for the 'MetNet2.py' docstring, would it be fine to change 'an even larger context' to 'a larger context' to reduce the length below 100 characters?

@jacobbieker
Copy link
Member

In the summary text for the 'MetNet2.py' docstring, would it be fine to change 'an even larger context' to 'a larger context' to reduce the length below 100 characters?

Yep, that sounds good!

@Averagenormaljoe
Copy link
Contributor Author

Averagenormaljoe commented Mar 28, 2025

Thank you, I added the remaining changes to the pull request.

@jacobbieker jacobbieker merged commit 6b306b0 into openclimatefix:main Mar 28, 2025
1 check passed
@Averagenormaljoe
Copy link
Contributor Author

Thank you for helping with the pull request. @jacobbieker

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.

3 participants