Skip to content

Conversation

@kandersolar
Copy link
Member

@kandersolar kandersolar commented Apr 16, 2025

  • Closes #xxx
  • Added tests to cover all new or modified code.
  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
  • Added new API functions to docs/api.rst.
  • Non-API functions clearly documented with docstrings or comments as necessary.
  • 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`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

reno was accidentally passing window_length to the infer_limits parameter. That means the window length was not being provided to the underlying pvlib function and instead the default was used. It also means that, since non-zero numbers are truthy, presumably this function has effectively had infer_limits=True.

@cwhanse should infer_limits=True be retained here?

@kandersolar kandersolar added the bug Something isn't working label Apr 16, 2025
@kandersolar kandersolar added this to the v0.2.3 milestone Apr 16, 2025
@kandersolar
Copy link
Member Author

python 3.8 CI failures should be ignored; we'll stop using 3.8 in #220.

@cwhanse
Copy link
Member

cwhanse commented Apr 16, 2025

should infer_limits=True be retained here?

I think infer_limits=True is better technically, but keeping that implied parameter would require fixing two other problems: 1) removing the explicit parameters calculated using scale_factor e.g. lower_line_length and 2) constraining window length to 30 minutes or less in this line:

window_length = np.minimum(10*delta_minutes, 60.0)

I vote we leave infer_limits out in this bug fix, and open an issue to advance detect_clearsky to default to infer_limits=True.

@kperrynrel your opinion? I mostly don't want to break any workflow for PVFleets.

@kperrynrel
Copy link
Member

@cwhanse @kandersolar thanks for checking in with us on this. Just took a look at our workflows and we're using the clearsky call associated with the Location() class in pvlib, which doesn't appear to use pvlib's clearsky.detect_clearsky() function so no issues expected on our end for changing this logic up. I'd vote for a separate PR for adding infer_limits since it's a modification and this is a bug fix.

@cwhanse
Copy link
Member

cwhanse commented Apr 17, 2025

@kperrynrel the pvlib Location.get_clearsky returns clearsky model GHI etc. for the location. Does PVFleets separate the PV system data by clear/nonclear condition? If so, how is that done?

@kperrynrel
Copy link
Member

@cwhanse we do not actually apply a clearsky filter in the routine. We do use the fixed_nrel() and tracking_nrel() functions in pvanalytics to identify sunny/clearsky days as a whole, and that is mainly what is used in lieu of any clearsky filter.

@cwhanse
Copy link
Member

cwhanse commented Apr 23, 2025

@kandersolar I think this PR is fine as is, or we could add a test to spy on pvlib's function call.

@kandersolar
Copy link
Member Author

In it goes!

@kandersolar kandersolar merged commit 861da05 into pvlib:main Apr 23, 2025
20 of 22 checks passed
@kandersolar kandersolar deleted the clearskybug branch April 23, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants