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

Update detect_clearsky( ) #1708

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Update detect_clearsky( ) #1708

wants to merge 32 commits into from

Conversation

eccoope
Copy link

@eccoope eccoope commented Mar 24, 2023

  • Closes Support for unequal time intervals #1678
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Modified algorithm in detect_clearsky( ) to support missing timestamps (in a DatetimeIndex of evenly spaced intervals). Supporting functions were also modified to accommodate this change.

@cwhanse cwhanse modified the milestones: 0.9.6, 0.9.5 Mar 29, 2023
pvlib/clearsky.py Outdated Show resolved Hide resolved
pvlib/clearsky.py Outdated Show resolved Hide resolved
pvlib/clearsky.py Outdated Show resolved Hide resolved
pvlib/clearsky.py Outdated Show resolved Hide resolved
pvlib/tests/test_tools.py Outdated Show resolved Hide resolved
@mikofski
Copy link
Member

Would it be possible to run the detect_clearsky benchmarks with and without this PR on the same machine for comparison?

@eccoope
Copy link
Author

eccoope commented Apr 18, 2023

Would it be possible to run the detect_clearsky benchmarks with and without this PR on the same machine for comparison?

Yes - just did this on my local machine. The old function takes 0.4933785000030184 seconds to run 10 times and the version in the PR takes 0.9015975999936927 seconds to do the same.

@kandersolar kandersolar modified the milestones: 0.9.6, 0.10.0 May 16, 2023
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.

IMO the performance hit is worth the enhancement of handling missing timesteps.

pvlib/clearsky.py Outdated Show resolved Hide resolved
2023-03-24 - This algorithm does accept data with skipped or missing
timestamps. The DatetimeIndex (either times or index of measured)
provided still must be regular, i.e. the length of intervals between
points are equal except in the case that data is missing.
"""

if times is None:
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this if and try should move down and use ispandas, optional cleanup.

# arrays of different lengths when evaluating comparison criteria and
# when indexing the Hankel matrix to construct clear_samples
elif len(clear_sky.index) != len(times):
clear = pd.Series(clear_sky, index=times)
Copy link
Member

Choose a reason for hiding this comment

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

Untested, and I suspect this will fail - clear_sky is a Series of different length than times so it can't get indexed by times.

Copy link
Author

Choose a reason for hiding this comment

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

Just added a test for this in test_clearsky.py

@kandersolar kandersolar modified the milestones: 0.10.0, v0.10.2 Jul 6, 2023
pvlib/tools.py Outdated
@@ -7,6 +7,7 @@
import pandas as pd
import pytz
import warnings
from .conftest import DATA_DIR
Copy link
Member

Choose a reason for hiding this comment

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

This line should be test_tools, that will fix the doc build error.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @cwhanse !

Move import statement to test_tools.py pt1
Move import statement to test_tools.py pt2
@kandersolar kandersolar modified the milestones: v0.10.2, v0.10.3 Sep 21, 2023
@kandersolar kandersolar modified the milestones: v0.10.3, v0.10.4 Dec 20, 2023
@kandersolar kandersolar modified the milestones: v0.10.4, Someday Mar 18, 2024
@RDaxini RDaxini mentioned this pull request Sep 26, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for unequal time intervals
4 participants