pvanalytics icon indicating copy to clipboard operation
pvanalytics copied to clipboard

Code and example for classifying snow effects

Open cwhanse opened this issue 10 months ago • 2 comments

  • [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.

cwhanse avatar Apr 08 '24 15:04 cwhanse

@kandersolar @kperrynrel I think we're far enough along for a first review, if you have time and interest.

cwhanse avatar May 16 '24 15:05 cwhanse

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.

cwhanse avatar Sep 27 '24 22:09 cwhanse

Test failure is in test_daytime.py; not caused by this PR. See https://github.com/pvlib/pvlib-python/issues/2238

kandersolar avatar Oct 04 '24 13:10 kandersolar

@kandersolar see if the example is reviewable now.

cwhanse avatar Oct 07 '24 20:10 cwhanse

thanks @kandersolar one last look or LGTY?

cwhanse avatar Oct 11 '24 21:10 cwhanse

Yes I think it's time to merge this. Thanks @eccoope and @cwhanse for making this PR happen!

kandersolar avatar Oct 16 '24 14:10 kandersolar