Support for PV systems with multiple arrays #1076
Conversation
Provides the three basic methods that depend on tilt and azimuth and provides a container for other parameters which may vary from array to array but do not vary within a single array (racking, temperature model params, albedo, strings, modules per string, etc.).
Removes array-related fields and adds _array field. All internal references to array-related attributes now refer to the the corresponding attributes of the Array class. It is likely some of the Array attributes will need to be exposed on the PVSystem instances via @properties; however, any of these instances are not covered by tests so it will be necessary to carefully work through the other places where PVsystem is used (ModelChain and child classes) to see what needs to be done.
|
The pvsystem.py test failures raise an issue: how much of the current API do we want to maintain? E.g., should |
|
And the second, closely-related issue: where to put the output of a Currently, It seems intuitive to store the results on |
|
I originally liked the idea of storing values like I think if we can decide what to do with methods like |
|
I'd rather not store any results on the objects. A few concerns: It doubles the scope of the API. I feel like maintainer burden scales faster than linear with API. Storing results on objects introduces a lot of state, which can make code a lot harder to reason about. ModelChain minimizes this problem with methods that largely compute consistent states. That wouldn't exist here without a lot of work. It's not clear to me how it would actually simplify the PVSystem/Array internal interfaces. |
|
Pushing on that thought - if no methods store results on |
|
Yes, I thought we had already agreed that the |
|
Maybe the least common denominator is to use lists and expect each It seems useful to group model output into a single |
Use the temperature model parameters from the Array instance inside PVSystem. Also properly initialize the parameters.
|
I'm going to keep pushing forward with a minimal working example of an |
Mistake in invocation of Array.__init__() in PVSystem.__init__()
Uses `@property` to provide getters and setters for Array attributes in order to mimic the original behavior of the PVSystem class.
|
@wfvining will |
Since the Array attributes are all exposed via @propertys on PVSystem, we can access them directly, reducing the number of changes needed to add the Array class.
|
@cwhanse I've written it for just one |
Adds the @singleton_as_scalar decorator to make a PVSystem with a single Array appear unchanged from the original PVSystem implementation.
Adds a decorator (@list_or_scalar) used to specify which parameters should be lists of the same length as PVSystem._arrays. The decorator handles validation by transforming non-list values to singleton lists then comparing to the length of the _arrays field on the PVSystem instance passed as the first parameter. Needs tests with multiple Arrays, but must wait until we have an API for adding multiple Arrays to a PVSystem.
This function is used in the test suite. It may not be needed otherwise, but to move forward without needing to rewrite the tests I'm adding it back to the PVSystem class as a wrapper around Array._infer_cell_type()
Make clear that the same model is applied to each array in the system.
This reverts commit 68617fa. The current sphinx configuration can't handle this because ModelChainResult does not have an explicit __init__ method. This results in an OSError during the build when sphinx tries to make the source code pages.
Removes redundant look-up in self.__dict__
Forgot to pass module_parameters to PVSystem constructor.
Requires slight modification of sphinx/source/conf.py to address getting line numbers for the ModelChainResult.__init__ method which is not explicitly defined (and therefore does not have a line number). Made the T TypeVar private so it does not clutter the attribute list for ModelChainResult. Add docstring for the PerArray type.
|
@kanderso-nrel Thanks for your review, it shook out some important stuff. I think I've addressed everything you mentioned if you want to take another look. |
Co-authored-by: Will Holmgren <[email protected]>
There was a problem hiding this comment.
Thanks @wfvining! I suggest opening an issue to flesh out the ModelChainResult page on RTD a bit more -- it already lists out the attributes, which is the important thing, but attribute descriptions would be nice too.
Some minor nits below, otherwise LGTM
| print(aoi) | ||
| system_multiarray.get_iam(aoi) |
There was a problem hiding this comment.
Very minor complaint -- this prints (array(0.99182856), array(0.99952705)); would be nice if it showed (0.99182856, 0.99952705) instead.
There was a problem hiding this comment.
Not sure how to achieve that without muddying the example. print(np.array(1)) will unwrap the value, but if you put arrays in a tuple you get the same result as above. We would need to resort to mapping str over the tuple, then printing it.
I guess tuple.__str__ calls __repr__ on its entries, not __str__. Seems like odd behavior to me, but what can you do.
| system_multiarray.get_iam(aoi) | |
| iam = system_multiarray.get_iam(aoi) | |
| print(tuple(map(str, iam))) |
There was a problem hiding this comment.
Ah I guess I was thinking that, if there was an issue to fix, it was that system.get_iam returned tuple of array instead of tuple of float, not that the printing here needs to be fixed. Let's not worry about this.
There was a problem hiding this comment.
Might be worth a separate issue to address the return types from the function layer.
|
I plan to merge this tomorrow unless I hear objections. |
|
Thanks @wfvining for the huge contribution and everyone else that contributed to the discussion and reviews! |
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/pvsystem.rstdocs/sphinx/source/whatsnewfor 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`).tupleorlistinputUpdate(See Update modelchain.rst #1115)modelchain.rstAddress unused(punting on this: Remove shadowed/unused kwargs from PVSystem methods #1118)kwargsinPVSystem.calcparams_xxxxandPVSystem.sapmAdd a
pvsystem.Arrayclass and update API to support a single PVSystem instance with multiple arrays. Incorporates the multi-array API in tomodelchain.ModelChain. Changes aim to be transparent to users who are modeling single-array systems, while the types of some fields (POA, DC power, etc) will change to a tuple for those who use the a PVSystem with multiple arrays.