Adds faiman_rad and ross models to get_cell_temperature().#2631
Adds faiman_rad and ross models to get_cell_temperature().#2631ramaroesilva wants to merge 18 commits intopvlib:mainfrom
Conversation
|
|
||
| temperature_cell = func(poa_global, temp_air, wind_speed, | ||
| *required, **optional) | ||
| if model == 'ross': |
There was a problem hiding this comment.
Although wind speed was kept as an input requested by get_cell_temperature, here such a differentiation is needed since ross is apparently the only model not using wind.
There was a problem hiding this comment.
@cwhanse @echedey-ls this if ross can be avoided if for the wind-using functions (all except ross), wind_speed is defined as input as below (example for one function):
# func = temperature.sapm_cell # to deprecate
func = functools.partial(temperature.sapm_cell, wind_speed=wind_speed)
and the if ross with two possibilities is replaced by the single line below
temperature_cell = func(poa_global, temp_air, *required, **optional)
|
Have included tests for the added temperature models. @adriesse, for the
Afterwards, I can get the expected output by running pvlib.temperature.faiman_rad for |
| * Add :py:func:`~pvlib.spectrum.spectral_factor_polo`, a function for estimating | ||
| spectral mismatch factors for vertical PV façades. (:issue:`2406`, :pull:`2491`) | ||
| * Includes `ross` and `faiman_rad` in the allowed models within | ||
| `pvlib.pvsystem.PVSystem.get_cell_temperature` (:issue:`2625`, :pull:`2631`) |
There was a problem hiding this comment.
| `pvlib.pvsystem.PVSystem.get_cell_temperature` (:issue:`2625`, :pull:`2631`) | |
| :py:meth:`pvlib.pvsystem.PVSystem.get_cell_temperature` (:issue:`2625`, :pull:`2631`) |
| MERRA-2 reanalysis data. (:pull:`2572`) | ||
| * Add :py:func:`~pvlib.spectrum.spectral_factor_polo`, a function for estimating | ||
| spectral mismatch factors for vertical PV façades. (:issue:`2406`, :pull:`2491`) | ||
| * Includes `ross` and `faiman_rad` in the allowed models within |
There was a problem hiding this comment.
| * Includes `ross` and `faiman_rad` in the allowed models within | |
| * Include `ross` and `faiman_rad` in the allowed models within |
|
|
||
| wind_speed : numeric | ||
| Wind speed [m/s] | ||
| Wind speed [m/s], although can be ``None`` for ``'ross'`` model |
There was a problem hiding this comment.
Should we add this to the corresponding PVSystem description too?
There was a problem hiding this comment.
do you mean within the function below?
@_unwrap_single_value
def get_cell_temperature(...)
There was a problem hiding this comment.
@kandersolar was this point blocking the merge of this PR for v0.14.0? Should have checked #2656 to provide more timely feedback.
There was a problem hiding this comment.
Not really. This PR is additive (i.e. not a breaking change), so I just didn't prioritize coming back to review it again in time for v0.14.0. We can get this PR into v0.14.1 no problem.
There was a problem hiding this comment.
do you mean within the function below?
I meant adding this same note about "can be zero for Ross" to this description as well: https://github.com/ramaroesilva/pvlib-python/blob/pvsystem-temp-models/pvlib/pvsystem.py#L429-L430
There was a problem hiding this comment.
This suggestion is still unresolved, I believe.
For the test it doesn't really matter what the values are, and what you've done seems fine. Not sure if you want to keep ir_down a scalar as discussed above. In practice the reduction in U values will depend on the ir_down conditions at the location. |
cwhanse
left a comment
There was a problem hiding this comment.
@ramaroesilva this fell off my radar, sorry. I don't have an opinion about the ross wind_speed accommodation. The if structure is OK with me, it's easy for a functools novice to understand :)
|
@adriesse @kandersolar nudging you if you wanted to look at this lingering PR again. |
| The irradiance that is converted to photocurrent in W/m^2. | ||
| Only used for some models. | ||
|
|
||
| ir_down: numeric, optional |
There was a problem hiding this comment.
| ir_down: numeric, optional | |
| ir_down: numeric or tuple of numeric, optional |
|
|
||
| wind_speed : numeric | ||
| Wind speed [m/s] | ||
| Wind speed [m/s], although can be ``None`` for ``'ross'`` model |
There was a problem hiding this comment.
This suggestion is still unresolved, I believe.
| func = functools.partial(temperature.faiman_rad, | ||
| ir_down=ir_down) | ||
| required = () | ||
| optional = _build_kwargs(['ir_down', 'u0', 'u1', |
There was a problem hiding this comment.
| optional = _build_kwargs(['ir_down', 'u0', 'u1', | |
| optional = _build_kwargs(['u0', 'u1', |
No need to include ir_down here now that it's passed in via the functools.partial.
I'm not familiar with the PVSystem class. |
|
|
||
| wind_speed : numeric | ||
| Wind speed [m/s] | ||
| Wind speed [m/s], although can be ``None`` for ``'ross'`` model |
There was a problem hiding this comment.
| Wind speed [m/s], although can be ``None`` for ``'ross'`` model | |
| Wind speed [m/s] | |
| When ``model='ross'``, this input is ignored |
Minor nitpick to bring more clarity to end users (similar to #1574)
docs/sphinx/source/referencefor API changes.docs/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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Differently from what was originally discussed with @cwhanse and @echedey-ls, I decided not to include a verification on whether both k and noct are provided within
get_cell_temperature. imo, this type of verification should be (and is) done within thetemperature.rossfunction itself and we see that the opposite situation - expected parameters not being provided - is never verified withinget_cell_temperatureas I imagine it is done within each temperature model function.@cwhanse about wind and
rossmodel, if that's okay I still included a small note in thewind_speeddescription to alertrossusers that providing aNoneis enough.