Skip to content

Updated pointers to current SAM files, updated column names, ... #80

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

Merged
merged 3 commits into from
Jul 16, 2015

Conversation

jforbess
Copy link
Contributor

and added more replace chars for equipment names [Issue 75].

The calcparams_desoto and sapm functions won't work with the previous csv files, because Sandia changed some column names between CSVs, and the change is hardcoded. It seems worth forcing the upgrade, though.

…d replace chars for equipment names [Issue 75]
@wholmgren
Copy link
Member

I'm not too worried about the name changes since I doubt that anybody is accessing those columns directly.

Is there any advantage to adding the ability to choose the data set?

elif name == 'sandiainverter':
url = 'https://sam.nrel.gov/sites/sam.nrel.gov/files/sam-library-sandia-inverters-2014-1-14.csv'
url = 'https://sam.nrel.gov/sites/sam.nrel.gov/files/sam-library-sandia-modules-2015-6-30.csv'
elif name == 'sandiainverter': # Still only one current inverter set, so left it as sandia, to avoid breaking old code
Copy link
Member

Choose a reason for hiding this comment

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

What if we do something like elif 'inverter' in name or elif name in ['sandiainverter', 'cecinverter'] to cover both the old code and the expectations of new users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't see much value in old versions, because if you are paying that close attention to the different module or inverter parameters, you would probably do better to have a local copy that you version yourself, and can specify directly as a file (and would have to update the column names to match, but that's not hard at this point).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, good points.

The doc string needs to be updated with something about how you can use either name and there is currently no difference between the two. I will merge after that.

@wholmgren
Copy link
Member

Oh, and add your change and name to the 0.2.1 whatsnew file.

@wholmgren wholmgren added this to the 0.2.1 milestone Jul 12, 2015
wholmgren added a commit that referenced this pull request Jul 16, 2015
Updated pointers to current SAM files, updated column names, ...
@wholmgren wholmgren merged commit a4460db into pvlib:master Jul 16, 2015
@wholmgren
Copy link
Member

Thanks @jforbess!

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

Successfully merging this pull request may close these issues.

2 participants