-
Notifications
You must be signed in to change notification settings - Fork 1.1k
rename series_modules, parallel_modules #176
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
Comments
Actually, I don't think the original terms were all that bad. At least there seemed little chance that they would be interpreted incorrectly, which is an important criterion. "modules_per_string" is fine, but I do not think "strings" is quite enough. Maybe "number_of_strings" could be considered, if long names are favoured. Personaly I prefer shorter ones, like "string_length" and "n_strings". Any of these new alternatives would seem imply one specific wiring scheme for the NxM matrix of modules, whereas the calculations for a homogenous system and conditions apply to all possible wiring schemes. |
I think it's assuming the same number of modules per string behind a single I think parallel_modules is confusing because in my head it should be On Fri, May 20, 2016 at 11:32 PM, Anton Driesse [email protected]
|
My vote is for Modules_per_string Cliff |
I like Cliff's suggestions. |
Yes, I think they are good. The only possible future proofing is Jessica On Thu, May 26, 2016 at 12:02 PM, Will Holmgren [email protected]
|
If we allow the possibiity of not having an inverter or an mppt unit, how about "strings_per_array"? |
But isn't the point to allocate a set amount of kWdc to a given mppt, so I was viewing the goal defining these parameters as the finest grain to be Even if it is not, I think that array is too general, and would mean On Fri, May 27, 2016 at 2:45 PM, Anton Driesse [email protected]
|
Yes, the word "array" is certainly open to interpretation. Feel free to |
At the risk of getting off topic... I think that at one point, maybe in #17, there was some consensus on defining a PVSystem as being a set of modules connected to a single inverter. It seemed like a fine place to start. But perhaps part of the reason for the disagreement/discussion here is that the existing PVSystem may not provide the right level of abstraction for many users. We may need a set of PVSystem-like classes to keep things reasonable. Off the top of my head, one related class could be PVPowerPlant, something that collects and aggregates multiple PVSystems. I don't see an obvious good solution for @jforbess's multi-mppt inverters using the existing PVSystem class. Let's make a new PVSystem refactoring issue if anyone wants to discuss this in more detail. Maybe we can also set aside some time at PVSC to discuss these ideas in person. That being said, I think that our existing PVSystem can model a lot of MWs with a simple modules_per_string and strings_per_inverter (assuming the same number of modules per string for all strings behind a single inverter). Long story short, I prefer to forge ahead with modules_per_string and strings_per_inverter for now because we may need to make much larger changes in the future. Is that reasonable? |
Yes, I think it's fine for now, considering the current state of the On Sun, May 29, 2016 at 12:29 PM, Will Holmgren [email protected]
|
Closed by #188. |
The
PVSystem
class and thesystem_def
function accept keyword argumentsseries_modules
andparallel_modules
. Until PR #171, pvlib code did not actually use them and they only existed for user extensions. As brought up in #171 by @jforbess, these names are confusing at best.Now is a good time to change them since they didn't actually do anything before. I would support something like
series_modules --> modules_per_string
andparallel_modules --> strings
.I'd normally wait for 0.4 to implement a change that could break user code, but in this case I'm leaning towards 0.3.3 release because 1) it's unlikely that anyone is actually using these arguments and 2) #171 is going in 0.3.3.
Thoughts?
The text was updated successfully, but these errors were encountered: