Skip to content

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Feb 19, 2019

  • Closes issue #xxxx
  • 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.

Adds a parser for the US Climate Reference Network files available at https://www.ncdc.noaa.gov/crn/qcdatasets.html

The map below shows the locations of these sensors.

screen shot 2019-02-18 at 7 41 22 pm

Question: what should we do about the columns that pvlib does not use (e.g. precip, soil moisture, surface temp)? I'm keeping them at the moment. I could add a kwarg, but I'd prefer to keep it simple unless there's a good reason to complicate it.

@wholmgren wholmgren added this to the 0.6.2 milestone Feb 19, 2019
@wholmgren wholmgren requested a review from cwhanse February 19, 2019 16:45
@wholmgren wholmgren added the solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter label Feb 19, 2019
@wholmgren wholmgren mentioned this pull request Feb 19, 2019
8 tasks
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.

LGTM although I don't understand fully how the test works, with the limited list of dtype included in the DTYPE constant. Would it be safer to specify type for every column?

@wholmgren
Copy link
Member Author

At first I only spec-ed the dtype for irradiance since that was the one that caused problems in the test, then I added the others that I actually cared about, but yes, better to just specify all of them. Latest commit also works around some pandas versions issues.

@wholmgren
Copy link
Member Author

I finally have all of the tests passing. I increased the minimum pandas version to 0.16 so that I could use Series.str.zfill and convert HMM format to HHMM format. Maybe there's another way, but this version of pandas did not remove much, so I don't think it's a problem.

@cwhanse
Copy link
Member

cwhanse commented Feb 19, 2019

About the non-solar fields: I think it's simpler to return the full CRN file content and let the pvlib user extract the data of interest for their application.

@wholmgren
Copy link
Member Author

Works for me.

I've found some issues with the read_fwf function default behavior. It seems that only the first line of the file is used to determine the field widths, so we'll need to hard code them. I actually did this in a prototype version of the code but removed them once I had the test data working without them. I'll expand the test data set to cover the problematic cases.

@wholmgren
Copy link
Member Author

GHI at Tucson AZ
crns0101-05-2019-az_tucson_11_w
Boulder CO
crns0101-05-2019-co_boulder_14_w
Oakley KS
crns0101-05-2019-ks_oakley_19_ssw
Socorro NW
crns0101-05-2019-nm_socorro_20_n

All times are UTC

I've looked at ~50 of ~150 sites and they all look reasonable in the first week of 2019.

@wholmgren
Copy link
Member Author

If there are no further comments, I think this is ready to merge.

@wholmgren
Copy link
Member Author

Thanks for the suggestions @mikofski!

@cwhanse cwhanse merged commit 437be18 into pvlib:master Feb 25, 2019
@wholmgren wholmgren deleted the crn branch February 25, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement io solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants