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.

Sign up for GitHub

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

Jump to bottom

Feature/add docstrings #197

Merged
merged 43 commits into from
Aug 10, 2024
Merged

Feature/add docstrings #197

merged 43 commits into from
Aug 10, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Jul 23, 2024 •
edited
Loading

Add docstrings, and docstring checks to FLASC

FLASC has functions currently missing docstrings, this is a problem given automatic documentation with sphinx relies on those strings.

Following FLORIS, we use the google style guide (https://google.github.io/styleguide/pyguide.html) but this is not uniformly followed so this PR tries to get higher uniformity

This pull request:

  • Updates RUFF to check for google-style docstrings
  • Makes running RUFF part of CI
  • Adds missing docstrings to functions
  • Makes all docstrings google-style compliant
    • Adds summary line
    • Focus on Args/Returns style
    • Only list type in return
    • Make sure all arguments appear in docstring
  • Add module level docstrings
  • Standardize on class / init docstrings a la: https://google.github.io/styleguide/pyguide.html
  • Fix spelling erros in docstrings where they appear

@paulf81 paulf81 added the documentation Improvements or additions to documentation label Jul 23, 2024
@paulf81 paulf81 self-assigned this Jul 23, 2024
@paulf81 paulf81 mentioned this pull request Jul 24, 2024
Open
@paulf81 paulf81 marked this pull request as ready for review July 25, 2024 00:11
@paulf81 paulf81 requested a review from misi9170 July 25, 2024 00:11
@paulf81
Copy link
Collaborator Author

paulf81 commented Jul 25, 2024

@misi9170 , this all grew a a bit as I got going and what small cheat I did was to add a few files that I wasn't sure we are keeping long term to the excluded list of docstring checks in pyproject.toml, but I think it's basically ready for review

@paulf81
Copy link
Collaborator Author

paulf81 commented Jul 26, 2024

Ok @misi9170 I made some final corrections think this now ready for review

misi9170
misi9170 requested changes Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@misi9170 misi9170 left a comment

Choose a reason for hiding this comment

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

@paulf81 here's a laundry list of little things to consider...

flasc/analysis/energy_ratio_output.py Outdated Show resolved Hide resolved
flasc/analysis/energy_ratio_output.py Outdated Show resolved Hide resolved
flasc/data_processing/energy_ratio_wd_bias_estimation.py Show resolved Hide resolved
flasc/data_processing/dataframe_manipulations.py Outdated Show resolved Hide resolved
flasc/analysis/energy_ratio_input.py Show resolved Hide resolved
flasc/utilities/energy_ratio_utilities.py Outdated
@@ -226,7 +220,6 @@ def add_wd_bin(
Returns:
pl.DataFrame: A new Polars DataFrame with an additional ws_bin column
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

flasc/utilities/floris_tools.py Show resolved Hide resolved
flasc/utilities/lookup_table_tools.py Show resolved Hide resolved
flasc/utilities/optimization.py Outdated Show resolved Hide resolved
flasc/visualization.py Show resolved Hide resolved
@paulf81
Copy link
Collaborator Author

paulf81 commented Jul 27, 2024

Thank you for these comments @misi9170 , I've tried to make all revisions but seems likely things could still be a little messy in places. I did find that the the saying a function returns a Tuple is the preferred google style so tried to update that everywhere.

@paulf81 paulf81 requested a review from misi9170 July 27, 2024 05:04
@misi9170
Copy link
Collaborator

misi9170 commented Jul 29, 2024 •
edited
Loading

@paulf81 Thanks. My feeling here is that we shouldn't necessarily aim for perfection with this PR, so I'm going to approve it.

Thanks for looking into the recommended style for the list of Returns. To be honest, I don't really agree with the recommendation of listing each multiargument return as a tuple---I think it's clearer to just list our each of the returns, as done with the Args, since as far as I'm aware, a tuple is always returned from a python function if there are multiple outputs.

As it stands, we have a mix of both of these styles for the Returns: statements. I'm OK to just leave it as is for now if you'd like, so that we can move forward with the PR.

@paulf81 paulf81 merged commit 4545f8a into NREL:develop Aug 10, 2024
3 checks passed
@paulf81 paulf81 deleted the feature/add_docstrings branch August 10, 2024 17:26
@misi9170 misi9170 mentioned this pull request Aug 13, 2024
Merged
misi9170 pushed a commit to misi9170/flasc that referenced this pull request Aug 13, 2024
@paulf81 @misi9170
Feature/add docstrings (NREL#197)
2d749aa
misi9170 added a commit that referenced this pull request Sep 4, 2024
@misi9170 @paulf81 @christiannvaughn
Add new FlascDataFrame object to enhance user interface with data (#…
bdc95e0 
…199)

* Adding at the root level for now; could consider moving to utilities/

* Ruff.

* precommit stuff.

* Add example notebook

* Add wide to long

* Add test

* Update README.md engagement

* Update README.md replacing 'FLORIS' typo

* Feature/add docstrings (#197)

* Update to 15 (#202)

* Update for PyPI installation. (#204)

* Update version number; ignore version.py for ruff.

* Adhere to FLASC formatting rules.

* psuedocode for conversions. Tests for saving dataframe to file.

* Print warning when to_feather used.

* String formatting.

* Add several more tests and n_turbines property.

* Adding TODOs for long/wide conversions.

* Clean up, a few more tests.

* Improved printout.

* ruff format.

---------

Co-authored-by: Paul <[email protected]>
Co-authored-by: christiannvaughn <[email protected]>
misi9170 added a commit that referenced this pull request Oct 17, 2024
@paulf81 @misi9170
@christiannvaughn @aclerc
Flasc dataframe (#212)
8456524 
* Add new `FlascDataFrame` object to enhance user interface with data (#199)

* Adding at the root level for now; could consider moving to utilities/

* Ruff.

* precommit stuff.

* Add example notebook

* Add wide to long

* Add test

* Update README.md engagement

* Update README.md replacing 'FLORIS' typo

* Feature/add docstrings (#197)

* Update to 15 (#202)

* Update for PyPI installation. (#204)

* Update version number; ignore version.py for ruff.

* Adhere to FLASC formatting rules.

* psuedocode for conversions. Tests for saving dataframe to file.

* Print warning when to_feather used.

* String formatting.

* Add several more tests and n_turbines property.

* Adding TODOs for long/wide conversions.

* Clean up, a few more tests.

* Improved printout.

* ruff format.

---------

Co-authored-by: Paul <[email protected]>
Co-authored-by: christiannvaughn <[email protected]>

* Add dataframe conversions to flascdataframe (#211)

* Add wind up example using `FlascDataFrame` (#210)

* loosen wind-up dependency

* Finish FlascDataFrame (Paul) (#215)

Add type hints

* Finish FlascDataFrame (update examples/docs) (#219)

* Merge recent changes.

* Update functions to use FlascDataFrames; getter and setter for channel_name_map; metadata copying method.

* Smarteole examples now using FlascDataFrame

* Add brief demonstration of switch to user data format; rename 09 notebook for clarity.

* Update artifical data examples.

* Had weird behavior in the getter/setter caused by not providing the true underlying attribute in _metadata.

* formatting.

* Simplify imports of FlascDataFrame throughout.

* Add wind-up to pyproject.toml after setup.py was removed.

* Update docs for FlascDataFrame (#220)

---------

Co-authored-by: misi9170 <[email protected]>
Co-authored-by: christiannvaughn <[email protected]>
Co-authored-by: Alex Clerc <[email protected]>
Co-authored-by: misi9170 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misi9170 misi9170 misi9170 approved these changes

Assignees

@paulf81 paulf81

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulf81 @misi9170