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

Clipping sphinx documentation #134

Merged
merged 12 commits into from Feb 24, 2022

Conversation

kperrynrel
Copy link
Collaborator

@kperrynrel kperrynrel commented Feb 21, 2022

Description

This PR generates documentation for the clipping mask in pvlib.features. I added an example data set labeled for clipping, and compare the results of the mask to the ground-truth labeled data (taken from the Duramat data sets site). Related to issue #133.

Checklist

The following items must be addressed before the code can be merged.
Please don't hesitate to ask for help if you are unsure of how to accomplish any of the items.
You are free to remove any checklist items that do not apply or add additional items that are
not on this list

  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings
  • Adds description and name entries in the appropriate "what's new" file
    in docs/whatsnew
    for all changes. Includes link to the GitHub Issue with :issue:`num`
    or this Pull Request with :pull:`num`. Includes contributor name
    and/or GitHub username (link with :ghuser:`user`).
  • Non-API functions clearly documented with docstrings or comments as necessary
  • Pull request is nearly complete and ready for detailed review

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

I'm not seeing a thumbnail for this example in the RTD PR build and I'm not sure why. It works when I build locally, so I'm inclined to chalk it up to browser strangeness and ignore it here.

The 4.7 MB data file is a bit large. Our current distribution is only 80-90 kB (https://pypi.org/project/pvanalytics/#files), so adding this would increase the size by 50x! What do you think about cutting down that data file to just the period used in this example and/or removing the unnecessary columns?

docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/whatsnew/0.1.2.rst Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
@kandersolar kandersolar added the documentation Improvements or additions to documentation label Feb 22, 2022
@kandersolar kandersolar added this to the v0.1.2 milestone Feb 22, 2022
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@cwhanse cwhanse 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 @kperrynrel . I would suggest using more descriptive legends, so that the two plots can be more easily identified as the labeled data and the detected clipping times.

docs/examples/clipping.py Outdated Show resolved Hide resolved
docs/examples/clipping.py Outdated Show resolved Hide resolved
kperrynrel and others added 2 commits February 23, 2022 09:12
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@kperrynrel
Copy link
Collaborator Author

@kanderso-nrel and @cwhanse made all the updates suggested. If you guys are good with it, I'll merge the PR into master. Thanks for the help with reviewing this!

@kperrynrel kperrynrel merged commit c72085d into pvlib:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants