pvlib.pvsystem.Array.get_cell_temperature does not accept ross and faiman_rad models

[ramaroesilva]

Is your feature request related to a problem? Please describe.
Users cannot choose the ross or faiman_rad as the value for the model parameter within the function mentioned in the title (source code here), although both appear to be fully implemented within pvlib.temperature (source).

Describe the solution you'd like
Add these options to the function by including the needed code blocks.

Additional context
I can do it myself, hopefully by sometime tomorrow (will create a PR and link it with this issue).

[markcampanelli]

Architecture question: I find it interesting that the temperature model is not declared/inferred when the Array object is first constructed. Are multiple temperature models actually accessed from Array objects in practice, leading to the need to pass the model parameter to get_cell_temperature? Furthermore, are the temperature-model parameters supposed to be mutable after object creation? If yes, then is that actually reasonable to accomplish?

[ramaroesilva]

For further context on my initial comment, I noticed this when working with pvlib.pvsystem.PVSystem.get_cell_temperature, which internally runs the same function but from the Array class (I guess to avoid redundancy, since a system is made by 1 or more arrays).

@markcampanelli about your comment, the advantage I see in specifying the model within the Array/System class would be for immediate internal checking on wheter the specified model and related parameters are compatible (e.g., making sure someone does not provide inputs for sapm but then uses ross). But I don't see why, on principle, all the other algorithmic choices are mutable but the module temperature isn't.

One possible advantage of the current setup would be that users can compare different models without having to redefine Array/System, but for that to happen all thermal-related parameters would have to be provided at once when setting up the system in the beginning.

[cwhanse]

I find it interesting that the temperature model is not declared/inferred when the Array object is first constructed.

Interesting idea. Probably, pvlib is the way it is because the order various pieces were added: first temperature models, then PVSystem, then ModelChain, finally Array. Temperature model parameters were attached to a PVSystem because that's where other parameters (e.g. module parameters) were attached. The design seems to have worked, but we should always be open to adjustments.

Users cannot choose the ross or faiman_rad

According to our own (new) temperature model guide, no reason these models shouldn't be options for get_cell_temperature. PR welcome.

[markcampanelli]

@ramaroesilva This is by no means a criticism of you wanting to fix the situation using the current design pattern. I am thankful that that you caught the inconsistency.

Bigger picture: This design pattern is found all over pvlib-python, and I personally take a much more controlled approach when designing with object-orientated programming (OOP). Carefully managing object state is so often overlooked in OOP. This leads to so many maintenance and extension headaches and (often rather subtle) mutated-state-related bugs. It also makes interfaces considerably harder to understand, especially when trying to attract and review new contributions to open source projects.

Whenever possible, I try to use a functional approach to transform data in numerical simulation/computing, but I will turn to OOP classes as containers to hold immutable data that are validated once upon object construction (for subsequent "safe" processing by the aforementioned pure functions). I claim that in the vast majority of cases the wins of immutability and not worrying about state management significantly outweigh the "flexibility" of allowing, but often not properly planning for, the complexity of mutable state.

All that said, I have also written OOP code that models real-world complex systems with considerable configuration state that must be validated, maintained, mutated, and ultimately used to produce output via method-based transformations of data that rely on said state configuration. I claim that doing this well for most non-trivial systems with high configurability is not nearly as "easy" as many OOP proponents claim.

[markcampanelli]

One other observation: It would be nice to take an interface approach to such temperature models (and many other model components), in which one only need to separately write a new temperature model according to said well designed and visible interface, and then one could expect to "drop in" this temperature model into the Array calculations, without having to update a long chain of elif model == "foobar" statements in the Array class itself. This design pattern, while harder to establish, is usually significantly easier to maintain and extend. (This pattern is usually called "dependency injection". It's gotta be one of the best "chess moves" out there for software design.)

[ramaroesilva]

@markcampanelli no criticism taken ;-)

And I fully understand your point. I have very little to no OOP experience and was only trying to point out advantages I could think of in both mutable and immutable options.

[ramaroesilva]

One other observation: It would be nice to take an interface approach to such temperature models (and many other model components), in which one only need to separately write a new temperature model according to said well designed and visible interface, and then one could expect to "drop in" this temperature model into the Array calculations, without having to update a long chain of elif model == "foobar" statements in the Array class itself. This design pattern, while harder to establish, is usually significantly easier to maintain and extend. (This pattern is usually called "dependency injection". It's gotta be one of the best "chess moves" out there for software design.)

@markcampanelli would having something like a YAML file listing the models and respective parameters and a list of models available within the pvlib code go in that direction? The list of models would check if users choose an available model and that would work, in turn, as a key to access the parameters within the YAML-read-as-dict variable.

[ramaroesilva]

Question: the get_cell_temperature function assumes wind_speed as a compulsory input. However, the ross function dismisses wind entirely, while faiman_rad assumes a default value of 1 (faiman as well btw).

Should allow users to not provide wind in these cases? Having wind assume a default value from start when calling get_cell_temperature raises one challenge and one requirement:

• changing the order of inputs (since the input following wind, model, has no default value) - seems a bit messy to not have all meteo inputs together
• if None as default, we would have to raise an error for non-ross cases

it would also require something like the code below in the end of this function

if wind_speed:
    temperature_cell = func(poa_global, temp_air,  wind_speed,
                                    *required, **optional)
else:    
    temperature_cell = func(poa_global, temp_air,
                                    *required, **optional)

[ramaroesilva]

Also, would appreciate external confirmation on this:

Within temperature.ross there are the k and noct parameters, both defined as optional (they have None as default value) but internally an error is raised if none or both are provided (users must provide one of the two). as such, I included the two in the optional side of the get_cell_temperature framework (the fact that one of the two is needed is stated in the doc and temperature.ross will handle this internally anyway through error messages).

[markcampanelli]

@markcampanelli would having something like a YAML file listing the models and respective parameters and a list of models available within the pvlib code go in that direction? The list of models would check if users choose an available model and that would work, in turn, as a key to access the parameters within the YAML-read-as-dict variable.

I was thinking of something that would only exist in code. I feel like I've gone through this example exercise before, but let me see if I can whip up an example for you.

[echedey-ls]

This is a broad topic, perfect for a discussion. You guys do have a point on improving ModelChain design. I also confess to doing my experimental local attempt to improve OOP in pvlib. Regarding proposals in this sense, I'm not sure if I rather see them in this repo or as completely new projects. This mismatch between features in pvlib(-core) and pvlib -modelchain is not uncommon. Many models make it into functional core but are not required in modelchain, to ease the contribution process.

My suggestion is to stick to ModelChain here and to open a discussion on pvlib OOP if that helps set up new goals and join efforts.

@ramaroesilva
Regarding

Within temperature.ross there are the k and noct parameters, both defined as optional (they have None as default value) but internally an error is raised if none or both are provided (users must provide one of the two).

Seems reasonable to me to either pass those two, and let the called function fail. Or to assert the length of _build_kwargs is one. I doubt there is any common use case where both are provided.

Question: the get_cell_temperature function assumes wind_speed as a compulsory input. However, the ross function dismisses wind entirely, while faiman_rad assumes a default value of 1 (faiman as well btw).

I really can't tell you any better solution. Maybe this would require a deprecation period (and a positional arg deprecation helper, maybe). Or just go ahead with requiring wind_speed anyway. It's easy to make it up if not available - thou it's not the most obvious approach for a newcomer. Even both ideas at the same time: require wind_speed and meanwhile add the positional arg deprecation. Better feedback can be provided when we see the full PR.

[cwhanse]

Can we move the ModelChain / OOP discussion to a Discussion? Here it complicates the discussion of adding ross to get_cell_temperature.

[cwhanse]

@ramaroesilva My thought is to leave wind_speed as required for get_cell_temperature. The burden is on the user to supply a value if model='ross' or 'faiman', which value is ignored so it does not seem a burden.

assert the length of _build_kwargs is one.

I like this idea.