Skip to content

Update detect_clearsky( ) - #1708

Open
eccoope wants to merge 32 commits into
pvlib:mainfrom
eccoope:cs_branch
Open

Update detect_clearsky( )#1708
eccoope wants to merge 32 commits into
pvlib:mainfrom
eccoope:cs_branch

Conversation

@eccoope

@eccoopeeccoope commented Mar 24, 2023

Copy link
Copy Markdown
  • ClosesSupport 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.

@cwhansecwhanse modified the milestones: 0.9.6, 0.9.5Mar 29, 2023
Comment threadpvlib/clearsky.py Outdated
Comment threadpvlib/clearsky.py Outdated
Comment threadpvlib/clearsky.py Outdated
Comment threadpvlib/clearsky.py Outdated
Comment threadpvlib/tests/test_tools.py Outdated
@mikofski

Copy link
Copy Markdown
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
Copy Markdown
Author

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.

@kandersolarkandersolar modified the milestones: 0.9.6, 0.10.0May 16, 2023
@kandersolarkandersolar mentioned this pull request May 16, 2023

@cwhansecwhanse left a comment

Copy link
Copy Markdown
Member

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.

Comment threadpvlib/clearsky.py Outdated
Comment threadpvlib/clearsky.py
points are equal except in the case that data is missing.
"""

if times is None:

Copy link
Copy Markdown
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.

Comment threadpvlib/clearsky.py Outdated
# 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
Copy Markdown
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
Copy Markdown
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

Comment threadpvlib/clearsky.py
@kandersolarkandersolar modified the milestones: 0.10.0, v0.10.2Jul 6, 2023
Comment threadpvlib/tools.py Outdated
import pandas as pd
import pytz
import warnings
from .conftest import DATA_DIR

Copy link
Copy Markdown
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
Copy Markdown
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
@kandersolarkandersolar mentioned this pull request Sep 6, 2023
@kandersolarkandersolar modified the milestones: v0.10.2, v0.10.3Sep 21, 2023
@kandersolarkandersolar modified the milestones: v0.10.3, v0.10.4Dec 20, 2023
@kandersolarkandersolar mentioned this pull request Mar 12, 2024
@kandersolarkandersolar modified the milestones: v0.10.4, SomedayMar 18, 2024
@RDaxiniRDaxini mentioned this pull request Sep 26, 2024
11 tasks
@AdamRJensen

Copy link
Copy Markdown
Member

@cwhanse is this still be worked on from Sandias side?

@cwhanse

Copy link
Copy Markdown
Member

@cwhanse is this still be worked on from Sandias side?

Let me talk a look at it. I had forgotten.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for unequal time intervals

5 participants

@eccoope@mikofski@AdamRJensen@cwhanse@kandersolar