pvanalytics
pvanalytics copied to clipboard
Code and example for classifying snow effects
- [x] Closes #200
- [x] Added tests to cover all new or modified code.
- [x] Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
- [x] Added new API functions to
docs/api.rst
. - [x] Non-API functions clearly documented with docstrings or comments as necessary.
- [x] 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`
). - [x] Pull request is nearly complete and ready for detailed review.
- [x] Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.
@kandersolar @kperrynrel I think we're far enough along for a first review, if you have time and interest.
Another partial review. High-level thoughts:
- The gallery example does too much. Simpler would be better.
Thanks for this. I needed an outside perspective, and I will overhaul the example to handle data from a single combiner.
- I am a little uncomfortable with the functions being only loosely tied to the reference (at least by pvlib-python standards). The code both extends the reference (e.g. adding a new mode) and fills in many gaps where the reference doesn't give specifics. It seems that these changes are necessary, so maybe all we can do is document the differences.
Agreed, but I'm not sure there's an option besides document here, or abandon the PR. A revised reference paper is very unlikely.
Test failure is in test_daytime.py
; not caused by this PR. See https://github.com/pvlib/pvlib-python/issues/2238
@kandersolar see if the example is reviewable now.
thanks @kandersolar one last look or LGTY?
Yes I think it's time to merge this. Thanks @eccoope and @cwhanse for making this PR happen!