Skip to content

Get clearsky.reno tests passing again #128

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.

Sign up for GitHub

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

Merged
kandersolar merged 3 commits into from
Jan 27, 2022
Merged

Get clearsky.reno tests passing again #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cd5024
fix #126
kandersolar Jan 27, 2022
09e93a0
fix #105
kandersolar Jan 27, 2022
51d1623
whatsnew
kandersolar Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/whatsnew/0.1.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ Bug Fixes
* Prohibit pandas versions in the 1.1.x series to avoid an issue in
``.groupby().rolling()``. Newer versions starting in 1.2.0 and older
versions going back to 0.24.0 are still allowed. (:issue:`82`, :pull:`118`)
* Fixed an issue with :py:func:`pvanalytics.features.clearsky.reno` in recent
pandas versions (:issue:`125`, :pull:`128`)

Contributors
~~~~~~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion pvanalytics/features/clearsky.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def reno(ghi, ghi_clearsky):

"""
delta = ghi.index.to_series().diff()
delta_minutes = delta[1] / np.timedelta64(1, '60s')
delta_minutes = delta[1].total_seconds() / 60
if delta_minutes > 15:
raise ValueError('clearsky requires regular time intervals '
'of 15m or less')
Expand Down
8 changes: 6 additions & 2 deletions pvanalytics/tests/features/test_clearsky.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
"""Tests for feature labeling functions."""
import pytest
import pandas as pd
import pvlib
from pvanalytics.features import clearsky

from pkg_resources import parse_version
is_old_pvlib = parse_version(pvlib.__version__) < parse_version("0.9.0")

@pytest.mark.skip(reason="GH #105")

@pytest.mark.skipif(is_old_pvlib, reason="GH #105")

@kandersolar kandersolar Jan 27, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is this sufficient for #105? It's not clear to me what the outcome of that issue was.

@cwhanse cwhanse Jan 27, 2022 •
edited
Loading

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'm OK with marking this test to be skipped, as a way to get beyond #105, but I think #105 is still open. IIRC, the action here is to replace the expected result in the test. The test was created using pvlib v0.8.1, which had a bug corrected in v0.9.0 via 1242.

@kandersolar kandersolar Jan 27, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What test values are there to update? These tests don't seem to use particularly precise hardcoded values anywhere, unless I'm missing something.

An aside: the commit message on 09e93a0 will automatically close #105, and I'm not sure how to prevent that now, so we'll have to manually reopen that issue if this is merged.

@cwhanse cwhanse Jan 27, 2022

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 test fails with pvlib v0.8.1. Looking back at stuff, there's nothing to fix here; the fix was made in pvlib v0.9.0. So I now think skipping the test with old pvlib closes #105

@pytest.mark.filterwarnings("ignore:Support for multi-dimensional indexing")
def test_reno_identical(quadratic):
"""Identical clearsky and measured irradiance all True"""
Expand All @@ -14,7 +18,7 @@ def test_reno_identical(quadratic):
assert clearsky.reno(quadratic, quadratic).all()


@pytest.mark.skip(reason="GH #105")
@pytest.mark.skipif(is_old_pvlib, reason="GH #105")
@pytest.mark.filterwarnings("ignore:Support for multi-dimensional indexing")
@pytest.mark.filterwarnings("ignore:invalid value encountered in")
def test_reno_begining_end(quadratic):
Expand Down