Skip to content

Change local_path to save_path in get_bsrn #1282

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
merged 7 commits into from
Sep 1, 2021
Merged

Change local_path to save_path in get_bsrn #1282

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8fc48df
Change local_path to save_path
AdamRJensen Aug 15, 2021
dc768e1
Update test_bsrn.py
AdamRJensen Aug 15, 2021
3cddc10
Update save_path description
AdamRJensen Aug 15, 2021
58b272c
Fix spelling mistake
AdamRJensen Aug 15, 2021
b661dcf
Test that file downloaded with save_path is read correctly
AdamRJensen Aug 25, 2021
cebfaf9
Update save_path description
AdamRJensen Aug 25, 2021
b083acd
Save testfile in temporary directory
AdamRJensen Sep 1, 2021
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
12 changes: 6 additions & 6 deletions pvlib/iotools/bsrn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def _empty_dataframe_from_logical_records(logical_records):


def get_bsrn(station, start, end, username, password,
logical_records=('0100',), local_path=None):
logical_records=('0100',), save_path=None):
"""
Retrieve ground measured irradiance data from the BSRN FTP server.

Expand All @@ -87,8 +87,8 @@ def get_bsrn(station, start, end, username, password,
logical_records: list or tuple, default: ('0100',)
List of the logical records (LR) to parse. Options include: '0100',
'0300', and '0500'.
local_path: str or path-like, optional
If specified, path (abs. or relative) of where to save files
save_path: str or path-like, optional
If specified, a directory path of where to save files.

Returns
-------
Expand Down Expand Up @@ -178,10 +178,10 @@ def get_bsrn(station, start, end, username, password,
# Check that transfer was successfull
if not response.startswith('226 Transfer complete'):
raise ftplib.Error(response)
# Save file locally if local_path is specified
if local_path is not None:
# Save file locally if save_path is specified
if save_path is not None:
# Create local file
with open(os.path.join(local_path, filename), 'wb') as f:
with open(os.path.join(save_path, filename), 'wb') as f:
f.write(bio.getbuffer()) # Write local file
# Open gzip file and convert to StringIO
bio.seek(0) # reset buffer to start of file
Expand Down
2 changes: 1 addition & 1 deletion pvlib/tests/iotools/test_bsrn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def test_get_bsrn(expected_index, bsrn_credentials):
station='tam',
username=username,
password=password,
local_path='')
save_path='')
Copy link
Member

@wholmgren wholmgren Aug 24, 2021

Choose a reason for hiding this comment

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

What happens when save_path=''? An empty string is not None (as the function logic tests for). Is this supposed to save a file?

Copy link
Member Author

@AdamRJensen AdamRJensen Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The idea is that save_path can either be a directory or file path (filename) depending on the nature of the function.

Since get_bsrn retrieves data from a number of actual files (and not a database), the original files are retrieves and saved to the specified directory path with their original filename.

save_path: str or path-like, optional
    If specified, a directory path of where to save files.

get_bsrn differs quite a bit from the other functions, as the other functions generally retrieve one request from an API (which doesn't have an associated file name) and then save_path would be a filename.

So in the get_bsrn case I assume the empty string represents the current working directory. I can definitely write a test that checks that (better yet modify the current test to check it).

Copy link
Member

@wholmgren wholmgren Aug 25, 2021

Choose a reason for hiding this comment

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

I guess it's ok here since the filename includes the station name and we're not matching existing API nor committing to retaining this API.

The idea is that save_path can either be a directory or file path (filename) depending on the nature of the function.

I doubt this will be clear to most users.

assert_index_equal(expected_index, data.index)
assert 'ghi' in data.columns
assert 'dni_std' in data.columns
Expand Down