Surfrad data reader #595

lboeman · 2018-10-02T23:24:33Z

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

Closes add SURFRAD data reader #590
I am familiar with the contributing guidelines.
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

stickler-ci · 2018-10-02T23:24:40Z

pvlib/iotools/surfrad.py

+        Dataframe with the fields found in SURFRAD_COLUMNS.
+    metadata: dict
+        Site metadata included in the file.
+


W293 blank line contains whitespace

stickler-ci · 2018-10-02T23:24:40Z

pvlib/test/test_surfrad.py

+import inspect
+import os
+
+from numpy import isnan


F401 'numpy.isnan' imported but unused

stickler-ci · 2018-10-02T23:24:40Z

pvlib/test/test_surfrad.py

+test_dir = os.path.dirname(
+    os.path.abspath(inspect.getfile(inspect.currentframe())))
+testfile = os.path.join(test_dir, '../data/surfrad-slv16001.dat')
+network_testfile ='ftp://aftp.cmdl.noaa.gov/data/radiation/surfrad/Alamosa_CO/2016/slv16001.dat'


E225 missing whitespace around operator
E501 line too long (96 > 79 characters)

stickler-ci · 2018-10-02T23:24:40Z

pvlib/test/test_surfrad.py

+
+@pytest.mark.parametrize('filename', [
+    testfile,
+    network_testfile ])


E202 whitespace before ']'

stickler-ci · 2018-10-02T23:24:41Z

pvlib/test/test_surfrad.py

+
+def test_format_index():
+    start = pd.Timestamp('20160101 00:00')
+    expected_index = pd.DatetimeIndex(start = start,


E251 unexpected spaces around keyword / parameter equals

stickler-ci · 2018-10-02T23:24:41Z

pvlib/test/test_surfrad.py

+def test_format_index():
+    start = pd.Timestamp('20160101 00:00')
+    expected_index = pd.DatetimeIndex(start = start,
+                                      periods = 1440,


E251 unexpected spaces around keyword / parameter equals

stickler-ci · 2018-10-02T23:24:41Z

pvlib/test/test_surfrad.py

+                 'latitude': '37.70',
+                 'longitude': '105.92',
+                 'elevation': '2317',
+                 'surfrad_version': '1' }


E202 whitespace before '}'

wholmgren

Nice work.

pvlib/iotools/surfrad.py

wholmgren · 2018-10-03T23:52:18Z

pvlib/iotools/surfrad.py

+    zen                  float   solar zenith angle (deg)
+    **Fields below have associated qc flags labeled <field>_flag.**
+    -----------------------------------------------------------------------
+    dw_solar             float   downwelling global solar(W/m^2)


I guess this could be dw_solar, ghi and you could add another ** line with a note like **raw, mapped fields are controlled by the map_variables argument**. There's probably a better way to say that.

wholmgren · 2018-10-04T00:14:37Z

pvlib/test/test_surfrad.py

+                    'Alamosa_CO/2016/slv16001.dat')
+
+
+def test_read_surfrad():


I don't think this test is doing anything for us so we can delete it

sorry for setting a bad example in test_tmy.py

pvlib/test/test_surfrad.py

wholmgren · 2018-10-04T00:19:59Z

pvlib/test/test_surfrad.py

+
+def test_format_index():
+    start = Timestamp('20160101 00:00')
+    expected_index = DatetimeIndex(start=start, periods=1440, freq='1min')


I think you can supply tz='UTC' here and skip the localize call below. Seems a little more straightforward to me. I had to look up if localize modifies in place or not.

Good catch, thanks. This was a naive "fix" when I was reading the test failure incorrectly, I wasn't setting the timezone correctly in read_surfrad.
It looks like pandas 0.14.0 has some issues with setting the timezone the way you proposed though, so i'll have to figure that out.

wholmgren · 2018-10-04T00:28:26Z

pvlib/test/test_surfrad.py

+
+def test_read_surfrad_metadata():
+    expected = {'name': 'Alamosa',
+                'latitude': '37.70',


the values here and below should be numeric. probably all floats.

wholmgren · 2018-10-04T16:23:56Z

Or we can update the minimum pandas requirement. Do you know what version we’d need to bump to?

…

On Thu, Oct 4, 2018 at 9:16 AM lboeman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pvlib/test/test_surfrad.py <#595 (comment)>: > ***@***.*** +def test_read_surfrad_network(): + surfrad.read_surfrad(network_testfile) + + +def test_read_surfrad_columns_exist(): + data, _ = surfrad.read_surfrad(testfile) + assert 'zen' in data.columns + assert 'temp' in data.columns + assert 'par' in data.columns + assert 'pressure' in data.columns + + +def test_format_index(): + start = Timestamp('20160101 00:00') + expected_index = DatetimeIndex(start=start, periods=1440, freq='1min') Good catch, thanks. This was a naive "fix" when I was reading the test failure incorrectly, I wasn't setting the timezone correctly in read_surfrad. It looks like pandas 0.14.0 has some issues with setting the timezone the way you proposed though, so i'll have to figure that out. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#595 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELiRxp2sqVbP3xNDhQpg3F7d7rg4IIbks5uhjRbgaJpZM4XFCSI> .

wholmgren · 2018-10-04T16:26:18Z

Yes. Good catch.

…

On Thu, Oct 4, 2018 at 9:25 AM lboeman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pvlib/iotools/surfrad.py <#595 (comment)>: > + from urllib.request import urlopen, Request + +import pandas as pd +import numpy as np + +SURFRAD_COLUMNS = [ + 'year', 'jday', 'month', 'day', 'hour', 'minute', 'dt', 'zen', + 'dw_solar', 'dw_solar_flag', 'uw_solar', 'uw_solar_flag', 'direct_n', + 'direct_n_flag', 'diffuse', 'diffuse_flag', 'dw_ir', 'dw_ir_flag', + 'dw_casetemp', 'dw_casetemp_flag', 'dw_dometemp', 'dw_dometemp_flag', + 'uw_ir', 'uw_ir_flag', 'uw_casetemp', 'uw_casetemp_flag', 'uw_dometemp', + 'uw_dometemp_flag', 'uvb', 'uvb_flag', 'par', 'par_flag', 'netsolar', + 'netsolar_flag', 'netir', 'netir_flag', 'totalnet', 'totalnet_flag', + 'temp', 'temp_flag', 'rh', 'rh_flag', 'windspd', 'windspd_flag', + 'winddir', 'winddir_flag', 'pressure', 'pressure_flag'] + Is 'zen': 'solar_zenith' also a valid mapping? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#595 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELiR8lb6bd_N_iz17B5Oou4k3FtWM-zks5uhjZdgaJpZM4XFCSI> .

lboeman · 2018-10-04T16:34:32Z

@wholmgren Looks like the tz parameter is added to pandas.DatetimeIndex in 0.15.0

wholmgren · 2018-10-04T16:58:53Z

Let's bump the minimum requirements to 0.15.0.

@mikofski will also appreciate the tz_localize(None) feature.

http://pandas.pydata.org/pandas-docs/stable/whatsnew.html#timezone-handling-improvements

pvlib/iotools/surfrad.py

lboeman · 2018-10-04T21:01:19Z

The bump to pandas 0.15.0 is causing other tests to fail and after testing it looks like all versions from 15-18.1 all produce errors in the tests. So it's probably best to roll this back to 0.14.0?

wholmgren

a few minor comments below. I'll check out the tests.

wholmgren · 2018-10-04T18:52:30Z

docs/sphinx/source/whatsnew/v0.6.1.rst

@@ -9,6 +9,8 @@ release.
 **Python 2.7 support will end on June 1, 2019**. Releases made after this
 date will require Python 3. (:issue:`501`)

+**Minimum pandas requirment bumped 0.14.0=>0.15.0**


requirement

wholmgren · 2018-10-04T18:53:43Z

pvlib/iotools/surfrad.py

+        Filepath or url.
+    map_variables: bool
+        When true, renames columns of the Dataframe to pvlib variable names
+        where applicable.


Add: "See variable SURFRAD_COLUMNS"

wholmgren · 2018-10-04T22:19:42Z

The min failures are due to a couple of issues.

test_clearsky and test_solarposition failures are solved with code suggested in #598. That should be addressed in a different PR, possibly #583.

test_pvsystem failures are due to pandas-dev/pandas#13006 and can be avoided by changing pvsystem.py lines 2651-2652 to:

    v_lim_upper = float(np.nanmax([v_max, vdc_max, mppt_hi]) * (1 + vtol))
    v_lim_lower = float(np.nanmax([v_min, mppt_low]) * (1 - vtol))

wholmgren · 2018-10-05T22:36:23Z

@lboeman can you merge the latest pvlib-python/master into your branch and make the changes to pvsystem.py proposed above? I think that should make the tests pass.

pvlib/pvsystem.py

lboeman · 2018-10-05T23:09:41Z

@wholmgren Updated those lines and all tests are passing now.

cwhanse · 2018-10-09T14:40:44Z

Just a thought, not requesting any change: the format_index function seems likely to be useful in other contexts. If I had an idea how to name it so that someone else would recognize its purpose from the name, I would suggest moving it to it.tools or something similar.

wholmgren · 2018-10-09T17:05:59Z

@cwhanse do you know of other data sources that use the SURFRAD time format? If not, I suggest we leave it as for now.

cwhanse · 2018-10-09T18:27:00Z

@wholmgren not off the top of my head. I agree, leave where it is for now, but let's move it if we find another iotools member function that parses this date format.

…iables option

wholmgren · 2018-10-09T19:54:14Z

docs/sphinx/source/whatsnew/v0.6.1.rst

+* Created :py:func:`pvlib.iotools.read_srml` and :py:func:`pvlib.iotools.read_srml_month_from_solardat`
+  to read University of Oregon Solar Radiation Monitoring Laboratory data. (:issue:`589`)
+
+* Created :py:func:`pvlib.iotools.read_surfrad` to read NOAA SURFRAD data. (:issue:`590`)


@lboeman your rebase introduced duplicate entries here

Thanks! Fixed.

stickler-ci bot reviewed Oct 2, 2018

View reviewed changes

wholmgren added enhancement solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter io labels Oct 2, 2018

wholmgren added this to the 0.6.1 milestone Oct 2, 2018

pvlib deleted a comment from stickler-ci bot Oct 3, 2018

wholmgren reviewed Oct 4, 2018

View reviewed changes

stickler-ci bot reviewed Oct 4, 2018

View reviewed changes

pvlib/iotools/surfrad.py Outdated Show resolved Hide resolved

pvlib/iotools/surfrad.py Outdated Show resolved Hide resolved

pvlib/iotools/surfrad.py Outdated Show resolved Hide resolved

wholmgren reviewed Oct 4, 2018

View reviewed changes

wholmgren mentioned this pull request Oct 4, 2018

improve solarposition.hour_angle and other analytical test functions #597

Closed

wholmgren mentioned this pull request Oct 5, 2018

add test for hour_angle, vectorize #599

Merged

8 tasks

lboeman force-pushed the surfrad branch from 2a75c4e to 35ae45e Compare October 5, 2018 22:48

stickler-ci bot reviewed Oct 5, 2018

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

wholmgren mentioned this pull request Oct 8, 2018

ENH: geometric calculations for sunrise, sunset, and transit #583

Merged

8 tasks

lboeman added 3 commits October 9, 2018 12:21

surfrad reader

e2f093b

minor updates

6e741de

coding style fixes

2fde993

lboeman added 8 commits October 9, 2018 12:21

DatetimeIndex updates for compatibility with pandas 0.14.0

af2aeaf

documentation updates

eef034c

Doc updates, addition to what's new and api

62b0021

bump min pandas version to 0.15.0, update tests and docs, add map_var…

3b34e38

…iables option

style fix

d844203

typo in whatsnew/doc update

756fa56

rebase, small fix to pvsystems.py

ecc9896

remove whitespace

c2eaecd

lboeman force-pushed the surfrad branch from 3f2eace to c2eaecd Compare October 9, 2018 19:24

wholmgren reviewed Oct 9, 2018

View reviewed changes

fix duplicate whatsnew entry after rebase

5c2e4ed

wholmgren merged commit 8058272 into pvlib:master Oct 9, 2018

mikofski mentioned this pull request Jan 30, 2021

SURFRAD site & date-range download #1155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surfrad data reader #595

Surfrad data reader #595

lboeman commented Oct 2, 2018 •

edited

Loading

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

stickler-ci bot Oct 2, 2018

wholmgren left a comment

wholmgren Oct 3, 2018

wholmgren Oct 4, 2018

wholmgren Oct 4, 2018

wholmgren Oct 4, 2018

lboeman Oct 4, 2018

wholmgren Oct 4, 2018

wholmgren commented Oct 4, 2018 via email

wholmgren commented Oct 4, 2018 via email

lboeman commented Oct 4, 2018

wholmgren commented Oct 4, 2018

lboeman commented Oct 4, 2018

wholmgren left a comment

wholmgren Oct 4, 2018

wholmgren Oct 4, 2018

wholmgren commented Oct 4, 2018

wholmgren commented Oct 5, 2018

lboeman commented Oct 5, 2018

cwhanse commented Oct 9, 2018

wholmgren commented Oct 9, 2018

cwhanse commented Oct 9, 2018

wholmgren Oct 9, 2018

lboeman Oct 9, 2018

Surfrad data reader #595

Surfrad data reader #595

Conversation

lboeman commented Oct 2, 2018 • edited Loading

pvlib python pull request guidelines

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

stickler-ci bot Oct 2, 2018

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Oct 4, 2018 via email

wholmgren commented Oct 4, 2018 via email

lboeman commented Oct 4, 2018

wholmgren commented Oct 4, 2018

lboeman commented Oct 4, 2018

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Oct 4, 2018

wholmgren commented Oct 5, 2018

lboeman commented Oct 5, 2018

cwhanse commented Oct 9, 2018

wholmgren commented Oct 9, 2018

cwhanse commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lboeman commented Oct 2, 2018 •

edited

Loading