Skip to content

move pvsystem.retrieve_sam to iotools #964

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

Open
wholmgren opened this issue May 15, 2020 · 7 comments
Open

move pvsystem.retrieve_sam to iotools #964

wholmgren opened this issue May 15, 2020 · 7 comments

Comments

@wholmgren
Copy link
Member

What do people think about moving pvsystem.retrieve_sam into the iotools subpackage? First brought up in #436.

@wholmgren wholmgren added the api label May 15, 2020
@kandersolar
Copy link
Member

ivtools is another option. I think of iotools as functions to handle i/o of timeseries weather data specifically. Although I suppose retrieve_sam fetches inverter parameters as well, so ivtools isn't a perfect fit either.

@cwhanse
Copy link
Member

cwhanse commented May 15, 2020

I'm in favor of moving it to iotools. I'd like to consider splitting that function into two, to read module and inverter parameters separately.

@CameronTStark CameronTStark added this to the 0.8.0 milestone May 15, 2020
@wholmgren
Copy link
Member Author

I'm not sure what the practical advantage of splitting the function by module/inverter would be. The API would remain the same in that you'd have to specify a string for the specific module/inverter database. I guess we could have different functions for each database.

pvlib-python/pvlib/pvsystem.py

Lines 1529 to 1547 in f8921bd

if name is not None:
name = name.lower()
data_path = os.path.join(
os.path.dirname(os.path.abspath(__file__)), 'data')
if name == 'cecmod':
csvdata = os.path.join(
data_path, 'sam-library-cec-modules-2019-03-05.csv')
elif name == 'sandiamod':
csvdata = os.path.join(
data_path, 'sam-library-sandia-modules-2015-6-30.csv')
elif name == 'adrinverter':
csvdata = os.path.join(data_path, 'adr-library-2013-10-01.csv')
elif name in ['cecinverter', 'sandiainverter']:
# Allowing either, to provide for old code,
# while aligning with current expectations
csvdata = os.path.join(
data_path, 'sam-library-cec-inverters-2019-03-05.csv')
else:
raise ValueError('invalid name {}'.format(name))

@wholmgren
Copy link
Member Author

@cwhanse do you have any further thoughts on how to refactor this function when moving to iotools?

The adrinverter is a bad fit for a function name that contains "SAM", too.

@cwhanse
Copy link
Member

cwhanse commented Jul 24, 2020

So far we've named iotools functions for the source of the data being read; that patterns suggests read_sam (I've never liked retrieve). I agree if we keep sam in the name it should only read files from sam, so adr would need it's own read function. We could do away with the name argument and return all the library files, just tossing that out.

An alternative, we break the pattern and align read functions to the models, which may be more easily recognized by users, e.g., read_cec_modules, than read_sam.

@toddkarin
Copy link

I like read_cec_modules better than read_sam. I'm indifferent on retrieve vs. read.

This would also be a great opportunity to rename parameters to standardization with pvterms.

@wholmgren
Copy link
Member Author

I like the consistency of read_ and the discoverability of e.g. _cec_modules. How about module names? Might need a couple of them depending on the name. Ideas: cec.py, adr.py, sandia.py, sam.py, module_inverter_parameters.py

I agree that it makes sense for new functions to return new keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants