search

pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.
https://pvlib-python.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.19k stars 998 forks source link

Update module and inverter files #761

Closed cwhanse closed 5 years ago

cwhanse commented 5 years ago

CEC module and inverter files are >1year old, and should be updated for the next pvlib release.

mikofski commented 5 years ago

The pvfree files are current as of February 2019.

Peque commented 5 years ago

Can I help with this? It seems like an easy issue to start contributing to this project.

Are the updated CEC module and inverter files available online somewhere? Do you already have code to fetch them automatically?

@mikofski I just had a quick look at your pvfree project but did not find any code to fetch the updated data/files. Maybe I just did not look hard enough?

mikofski commented 5 years ago

just had a quick look at your pvfree project but did not find any code to fetch the updated data/files.

There's nothing currently in there, but it would be great to add. If you're interested in creating a PR, either here or there, it could be very useful. For example, if you add this functionality to pvlib.iotools then pvfree could add a button, and call that routine

Peque commented 5 years ago

@mikofski How do you currently update the data? (i.e.: where and how do you fetch the information from) Do you have even a very basic script to start with or is it a completely manual process?

Even if it is manual, I do not currently know where am I supposed to fetch the updated data from. Is it a website? a database? a set of PDFs? where are they located? are they freely available? :blush:

mikofski commented 5 years ago

I use the csv files in the NREL SAM deploy/libraries. On the pvfree homepage there's an upload button under admin that I use. Since I'm on the free heroku plan, I have no workers to do async, so if the file is too big I have to chunk it up, or the upload will time out. For one off, I can just use the API with my API key, but I've never actually done it except as a test, b/c we have no governance policy yet for who and what can be uploaded.

@cwhanse and @cpaulgilman have some scripts to retrieve the bulk xlsx files from CEC go solar CA website, so perhaps start there?

Thanks!!!

cwhanse commented 5 years ago

@Peque Thanks for the offer. The parameters used by PV performance models in pvlib are not in the CEC xlsx files. The parameters are estimated using code in SAM and published in files included with SAM here.

We are talking offline with NREL about how to make the parameter process public. pvfree may be part of that solution but until we are agreed on a path forward let's hold off on adding code to pvlib.iotools.

Peque commented 5 years ago

@cwhanse Oh, I see. I will wait until you reach an agreement.

Please, keep us updated on this thread when you do! :blush:

cwhanse commented 5 years ago

@Peque it appears I wasn't clear. pvlib presently uses the csv files published with SAM at the links @mikofski and I included. This issue is open to remind us to update pvlib with the latest copy from SAM. If you want to make a PR to replace the two csv files, that is very welcome, and thank you in advance.

I want to hold off building a link between pvlib and pvfree's parameter service until the offline conversation determines that code is needed.

Peque commented 5 years ago

@cwhanse Oh, I see. :blush:

I opened https://github.com/pvlib/pvlib-python/pull/767. I noted the previous CSV files were exact copies of those fetched from SAM, so I just did the same: replace the old data files with the new ones as they can be fetched from SAM. That means the columns have slightly changed (there are new columns and the order may not be the same).

Peque commented 5 years ago

It seems keys have changed as well. In example: ABB: MICRO-0.25-I-OUTD-US-208 208V [CEC 2014], is now known as ABB: MICRO-0.25-I-OUTD-US-208 [208V]. So this change would not be backwards-compatible (i.e.: old code would need to be updated with the new keys).

How do you want to deal with that? Should I just change the keys in the tests and break backwards compatibility?

If so maybe we could take the chance and, at the same time, avoid replacing so many characters with _, as suggested by @wholmgren . Which is another backwards-incompatible change.

cwhanse commented 5 years ago

@cwhanse Oh, I see. 😊 I opened #767. I noted the previous CSV files were exact copies of those fetched from SAM, so I just did the same: replace the old data files with the new ones as they can be fetched from SAM. That means the columns have slightly changed (there are new columns and the order may not be the same).

That's the hope - copy the SAM file and make it work in pvlib. New columns and changed column order shouldn't matter.

cwhanse commented 5 years ago

It seems keys have changed as well. In example: ABB: MICRO-0.25-I-OUTD-US-208 208V [CEC 2014], is now known as ABB: MICRO-0.25-I-OUTD-US-208 [208V]. So this change would not be backwards-compatible (i.e.: old code would need to be updated with the new keys).

@janinefreeman @cpaulgilman was the change in module/inverter name made by the CEC? Wondering if SAM maintained an association between old and new names for compatibilty.

How do you want to deal with that? Should I just change the keys in the tests and break backwards compatibility?

I think we can change the hardwired keys in the tests (there are maybe 5?) but we may have to fix the expected test outcomes. It would be good to learn that in thsi PR.

If so maybe we could take the chance and, at the same time, avoid replacing so many characters with _, as suggested by @wholmgren . Which is another backwards-incompatible change.

My view is to do the minimum work necessary to update to the new SAM files, given that we hope to work with NREL and others to build a more reliable and open system for developing and delivering the parameters. @wholmgren @mikofski

wholmgren commented 5 years ago

My view is to do the minimum work necessary to update to the new SAM files, given that we hope to work with NREL and others to build a more reliable and open system for developing and delivering the parameters

Probably best to leave the underscores as they are at this point.

As a reminder, the retrieve_sam function accepts a path parameter that may be a URL to a remote file. Perhaps we should just stop bundling the database with pvlib. We could also consider a function cache_sam_data that gets and stores the data in a local cache in a hidden directory like ~/.pvlib. retrieve_sam would check the cache before going to the remote URL.

cwhanse commented 5 years ago

As a reminder, the retrieve_sam function accepts a path parameter that may be a URL to a remote file. Perhaps we should just stop bundling the database with pvlib. We could also consider a function cache_sam_data that gets and stores the data in a local cache in a hidden directory like ~/.pvlib. retrieve_sam would check the cache before going to the remote URL.

I agree with pointing to a URL as a long term solution, but the current SAM files are a risk, as illustrated by this PR discovering format changes that break tests and compatibility with previous pvlib versions. I'd rather not have those issues discovered at run time.

Peque commented 5 years ago

I created separate issues for things mentioned here:

Last one was not mentioned, but wanted to add it. :innocent: Feel free to close it if you do not agree with the proposed change.

Peque commented 5 years ago

@cwhanse Some tests fail after updating the keys (see https://github.com/pvlib/pvlib-python/pull/767 pipelines). So I guess data was changed as well, not only names.

There is one module in particular: Example_Module that does not have a match in the new database (or I have not found it). How do you want to deal with that?

cwhanse commented 5 years ago

@cwhanse Some tests fail after updating the keys (see #767 pipelines). So I guess data was changed as well, not only names. There is one module in particular: Example_Module that does not have a match in the new database (or I have not found it). How do you want to deal with that?

I had no idea that this update would become a substantial amount of work. Another reason to separate the module parameter file from pvlib code.

Since the module parameter values themselves have changed, it appears that we should lock down the old parameter sets in some manner, either hardcoding the parameters into the test_* files, or creating a CSV files with only the old parameter sets used by the tests and reading the file with retrieve_sam instead of reading the full SAM file. I lean toward the 2nd option, because the purpose is not to test parameter reading, but rather the models that use the parameters. @wholmgren what do you think? Example Module is an entry in the SAM file in the current pvlib. I don't know when SAM dropped that entry.

wholmgren commented 5 years ago

There are only a handful of parameter sets used in the test code, so I would lean towards hard coding them. So, within the tests retrieve_sam should only be called in the function(s) like test_retrieve_sam but nowhere else.

Peque commented 5 years ago

@cwhanse @wholmgren I opened a PR (https://github.com/pvlib/pvlib-python/pull/774) just to discuss the approach. If you like it, I can do the same for the CEC modules. Then we could proceed with updating the SAM CSV files.

Note I changed one test: test_pvsystem.py:test_snlinverter_Pnt_micro. It was using an Enphase Energy inverter for the test but, to me, it seemed like the test could use the ABB microinverter as well.

wholmgren commented 5 years ago

Note I changed one test: test_pvsystem.py:test_snlinverter_Pnt_micro. It was using an Enphase Energy inverter for the test but, to me, it seemed like the test could use the ABB microinverter as well.

I can't recall the specifics, but we added one of these when fixing a bug caused by some particular parameter or combination of parameters. We should keep both or go back to the old issue/pr to understand if it's really safe to remove one.

Peque commented 5 years ago

@wholmgren Is the approach fine with you? (I would like to know before continuing) :blush:

(I mean without taking into account that change, which I will probably revert)

Peque commented 5 years ago

@wholmgren Yeah, you were right (https://github.com/pvlib/pvlib-python/issues/140).

I reverted the changes and added a short docstring to the test function (https://github.com/pvlib/pvlib-python/pull/774/commits/b9fa72d6a566cc345b2f4764c560caea3d079f23). Hope you do not mind, even if the rest of the tests in the file are missing docstrings. :innocent:

janinefreeman commented 5 years ago

Sorry for the delayed response: we try to do some name checking when we process the library from the CEC, but they periodically drop modules and we did make the decision that we don't want to try to append the list with new modules, because it's next to impossible to tell what's changed under current naming schemes. So we pretty much process what they give us and publish it in SAM- which does occasionally mean that components get dropped. Hopefully with the GUID approach that @cwhanse says Orange button is hoping to develop, we can avoid some of these problems when we switch to a new repository...