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

Add retrieval function for BSRN data #1254

Merged
merged 33 commits into from
Jul 30, 2021
Merged

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Jul 5, 2021

  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

Add a retrieval function for BSRN data pvlib.iotools.get_bsrn. Data is obtained from the BSRN's FTP server.

@AdamRJensen
Copy link
Member Author

@kanderso-nrel This is my initial idea for the get_bsrn function.

The read_bsrn function needs to be updated to take a file buffer or a parse_bsrn function needs to be created.

@wholmgren
Copy link
Member

@AdamRJensen our solarforecastarbiter-core adaptation of the BSRN code that you contributed to pvlib includes a change to make a parse_bsrn function that only takes a file buffer. Maybe that would be helpful to copy back over here.

@AdamRJensen
Copy link
Member Author

@kanderso-nrel Any idea of why the Linux versions fail, but the others pass?

The log states that the test test_get_bsrn_no_files is failing on Linux due to "Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) was emitted."

My knowledge of Linux is too limited to understand why it would fail on Linux and not the others.. any ideas?

@kandersolar
Copy link
Member

@kanderso-nrel Any idea of why the Linux versions fail, but the others pass?

I don't think it's actually a linux-specific issue, it's just that only the conda_linux versions are configured to run the remote_data tests. I can recreate the CI failure on a local ubuntu VM. The problem is that it actually does find a file for the bogus date -- I get back data for the month of 2000-06. I guess the issue is that files are named with the format f"{station}%m%y.dat.gz", so year 1800 gets formatted as year 00 and interpreted as year 2000. Changing the start and end from 1800 to 1899 gets the test to pass for me.

@AdamRJensen
Copy link
Member Author

AdamRJensen commented Jul 22, 2021

@cwhanse Any opinion on adding "gri" (ground-reflected irradiance) to the pvlib list of variables? Or perhaps there are better alternatives for the naming of upward short-wave reflected irradiance?

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

This looks great @AdamRJensen! Comments below, mostly very nitpicky...

pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
docs/sphinx/source/whatsnew/v0.9.0.rst Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/iotools/bsrn.py Outdated Show resolved Hide resolved
pvlib/tests/iotools/test_bsrn.py Outdated Show resolved Hide resolved
pvlib/tests/iotools/test_bsrn.py Show resolved Hide resolved
@AdamRJensen
Copy link
Member Author

Thanks for the review @kanderso-nrel! Do we need a second review on this?

@kandersolar
Copy link
Member

Do we need a second review on this?

My answer to that question is always yes :) https://en.wikipedia.org/wiki/Linus%27s_law

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

LGTM, @pvlib/pvlib-maintainer anyone else want to review this?

@cwhanse
Copy link
Member

cwhanse commented Jul 26, 2021

LGTM, @pvlib/pvlib-maintainer anyone else want to review this?

I don't see how I will make time for it. I vote merge and improve later, if needed.

@wholmgren wholmgren mentioned this pull request Jul 30, 2021
24 tasks
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Looks good to me. I confirmed the parse function works with data I obtained from the NASA Langley site using requests.

The lines that are not covered are either expected due to the test configuration or are ok to skip for iotools.

Thanks @AdamRJensen! I expect this will be useful to many people.

@wholmgren wholmgren added this to the 0.9.0 milestone Jul 30, 2021
@wholmgren wholmgren merged commit 7766bc6 into pvlib:master Jul 30, 2021
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.

4 participants