-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Adds k parameter to temperature.ross #2521
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
base: main
Are you sure you want to change the base?
Conversation
In #2506 @cwhanse mentions that it is difficult to find the 1981 paper from Ross, to which @kandersolar provided a wayback machine url. Should I add it to the documentation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @ramaroesilva
I added a few suggestions for formatting the docstring and references. Have a look, and if you look under the "files changed" tab you can "add suggestion to batch" and implement more than one suggestion in a single commit
noct : numeric | ||
Nominal operating cell temperature [C], determined at conditions of | ||
800 W/m^2 irradiance, 20 C ambient air temperature and 1 m/s wind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to keep PR scope focused, but our units convention was only recently introduced and we aim to update the documentation gradually. Since you're working on this function anyway, I don't think anyone would object to (personally, I would encourage) updating the formatting of these units here and elsewhere in this function. That said, anyone, feel free to object
noct : numeric | |
Nominal operating cell temperature [C], determined at conditions of | |
800 W/m^2 irradiance, 20 C ambient air temperature and 1 m/s wind. | |
noct : numeric | |
Nominal operating cell temperature [°C], determined at conditions of | |
800 Wm⁻² irradiance, 20 °C ambient air temperature and 1 ms⁻¹ wind speed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the initiative @RDaxini. I thought about that but then I noticed that the issue applies to the whole file. So to make the PR more focused, I kept the original formatting and moved that to a separate issue (#2519).
If that's okay for you, I would leave this PR with the Adds
» Add
and correct the doi formatting (which I've already commited).
Next week I will address the unit styling in #2519, feel free to join the discussion. There I'm proposing to have a .
between units (e.g. W.m⁻²
) to make reading easier, and to present the Ross equation with 800
in the denominator to shift from the historically cell-motivated mW.cm⁻² to a more module-friendly W.m⁻² (which also dismisses the current 0.1
factor to match the irradiance unit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's do edits for style in #2519 for all of temperature.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is already work planned to do this separately then that's fine
@@ -638,6 +638,9 @@ def ross(poa_global, temp_air, noct): | |||
noct : numeric | |||
Nominal operating cell temperature [C], determined at conditions of | |||
800 W/m^2 irradiance, 20 C ambient air temperature and 1 m/s wind. | |||
k: numeric | |||
Ross coefficient [Km^2/W], which is an alternative to employing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ross coefficient [Km^2/W], which is an alternative to employing | |
Ross coefficient [Km²W⁻¹], which is an alternative to employing |
where :math:`S` is the plane of array irradiance in :math:`mW/{cm}^2`. | ||
This function expects irradiance in :math:`W/m^2`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let others comment, but I think Wm⁻²
looks cleaner than the larger font bolt+italic math type for units. Might be worth saving that for variables(?)
At least if sticking with :math:
then I think superscript would be better than a /
: `:math:`mW{cm}^{-2}
where :math:`S` is the plane of array irradiance in :math:`mW/{cm}^2`. | |
This function expects irradiance in :math:`W/m^2`. | |
where :math:`S` is the plane of array irradiance in mWm⁻². | |
This function expects irradiance in Wm⁻². |
pvlib/temperature.py
Outdated
Energy, vol. 34, no. 1, pp. 23–29, Jan. 2009, doi: | ||
https://doi.org/10.1016/j.renene.2008.04.009. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the sphinx doi role here to hyperlink the doi to the paper automatically
Energy, vol. 34, no. 1, pp. 23–29, Jan. 2009, doi: | |
https://doi.org/10.1016/j.renene.2008.04.009. | |
Energy, vol. 34, no. 1, pp. 23–29, Jan. 2009, | |
:doi:`10.1016/j.renene.2008.04.009` |
* Rename parameter name ``aparent_azimuth`` to ``solar_azimuth`` in :py:func:`~pvlib.tracking.singleaxis`. | ||
(:issue:`2479`, :pull:`2480`) | ||
* Adds k coefficient in :py:func:`~pvlib.temperature.ross` | ||
(:issue:`2506`, :pull:`2521`) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds
--> Add
for the whatsnew
* Rename parameter name ``aparent_azimuth`` to ``solar_azimuth`` in :py:func:`~pvlib.tracking.singleaxis`. | |
(:issue:`2479`, :pull:`2480`) | |
* Adds k coefficient in :py:func:`~pvlib.temperature.ross` | |
(:issue:`2506`, :pull:`2521`) | |
* Rename parameter name ``aparent_azimuth`` to ``solar_azimuth`` in :py:func:`~pvlib.tracking.singleaxis`. | |
(:issue:`2479`, :pull:`2480`) | |
* Add k coefficient in :py:func:`~pvlib.temperature.ross` | |
(:issue:`2506`, :pull:`2521`) | |
pvlib/temperature.py
Outdated
|
||
where :math:`S` is the plane of array irradiance in :math:`mW/{cm}^2`. | ||
This function expects irradiance in :math:`W/m^2`. | ||
|
||
Reference values for k are provided in [2]_, covering different types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference values for k are provided in [2]_, covering different types | |
Representative values for k are provided in [2]_, covering different types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this in latest commit, thanks!
pvlib/temperature.py
Outdated
+---------------------+-----------+ | ||
| Mounting | :math:`k` | | ||
+=====================+===========+ | ||
| well_cooled | 0.02 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely optional: you could replace the underscore with a space, because pvlib isn't providing these values in a dict with e.g. well_cooled
as a key value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed your suggestion in latest commit, thanks!
pvlib/temperature.py
Outdated
# factor of 0.1 converts irradiance from W/m2 to mW/cm2 | ||
return temp_air + (noct - 20.) / 80. * poa_global * 0.1 | ||
if (noct is None) & (k is None): | ||
raise ValueError("noct or k need to be provided as numeric input.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError("noct or k need to be provided as numeric input.") | |
raise ValueError("Either noct or k is required.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these new warnings need to be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pvlib/temperature.py
Outdated
if (noct is None) & (k is None): | ||
raise ValueError("noct or k need to be provided as numeric input.") | ||
elif (noct is not None) & (k is not None): | ||
raise ValueError("Provide only noct or k, not both.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError("Provide only noct or k, not both.") | |
raise ValueError("Provide only one of noct or k, not both.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this in latest commit, thanks!
noct : numeric | ||
Nominal operating cell temperature [C], determined at conditions of | ||
800 W/m^2 irradiance, 20 C ambient air temperature and 1 m/s wind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's do edits for style in #2519 for all of temperature.py.
return temp_air + (noct - 20.) / 80. * poa_global * 0.1 | ||
elif noct is None: | ||
# k assumes irradiance in W.m-2, dismissing 0.1 factor | ||
return temp_air + k * poa_global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing: please check that the function returns correct values when k
is passed. Please test with input float
, np.array
and pd.Series
. Also, please test that both ValueError
are raised when the conditions are met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the types you mention refer to poa_global
and air_temperature
all seems good. For noct
and k
I've tested int
the first and float
for the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the tests to test_temperature.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RDaxini tests added :-)
@ramaroesilva if you can fix the merge conflicts in the whatsnew file, we can launch the CI. To fix the conflicts, just keep both sets of lines - this sometimes happens when two PRs are adding lines at the same position. |
@cwhanse merge conflict fixed :-) |
if (noct is None) & (k is None): | ||
raise ValueError("Either noct or k need is required.") | ||
elif (noct is not None) & (k is not None): | ||
raise ValueError("Provide only one of noct or k, not both.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The codecov checks on this PR fail because these ValueError
s are not covered
You can address this using with pytest.raises():
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you be a bit more explicit? Never used that before.
Also, fyi, there is a similar raise ValueError
in the function noct_sam
in this same file. Should it also be changed?
|
||
expected = expected.values | ||
assert_allclose(expected, result1) | ||
assert_allclose(expected, result2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for the errors, to raise the code coverage.
assert_allclose(expected, result2) | |
assert_allclose(expected, result2) | |
def test_noct_sam_errors(): | |
with pytest.raises(ValueError, match='Either noct or k need is required'): | |
temperature.ross(1000., 30.) | |
with pytest.raises(ValueError, match='Provide only one of noct or k'): | |
temperature.ross(1000., 30., noct=45., k=0.02) |
You can ignore the test failure for py3.9 |
+---------------------+-----------+ | ||
| On sloped roof | 0.0563 | | ||
+---------------------+-----------+ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we'll be asked what is meant by some of these descriptors: "not so well cooled", "on sloped roof" - is that a sloped roof, or modules tilted up away from the roof? I've got a copy of the paper that [2] references, I'll leave a comment with suggested text to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On sloped roofs:
Well cooled: On a sloped roof, gap between modules and the mounting surface allows for convective cooling.
Not so well cooled: On a sloped roof, air flow is restricted between the modules and the mounting surface.
On flat roofs:
Free standing: On a flat roof, gap between modules and the mounting surface allows for convective cooling.
Flat on roof: On a flat roof, air flow is restricted between the modules and the mounting surface.
Building integrated:
On sloped roof: modules integrated with the roof/mounting surface, without air flow cooling the underside.
Facade integrated: opaque modules integrated with a facade, with air flow cooling the inner surface of the mounting structure.
Transparent PV: I have no idea from [2] or [3] what this describes. transparent modules integrated with a facade, with air flow cooling the inner surface of the mounting structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your effort @cwhanse and for pointing out the unclear designations!
I disagree with the author flat on roof designation, which I believe misled your interpretation. The original reference mentions
Freestanding and flat roof systems usually allow a free airflow around the modules and have therefore lower temperature losses.
This makes me think both are tilted modules, but one on the ground and another on top of a flat roof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think both are tilted modules, but one on the ground and another on top of a flat roof.
I concur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead I would propose a new, more self-explanatory nomenclature, mentioning the original reference as inspiration (with the values made explicit in our documentation, it should be easy for people to make the correspondance with the Skoplaki publication).
Well cooled » Sloped roof, well ventilated
Free standing » Free-standing system
Flat on roof » Flat roof, well ventilated
Not so well cooled » Sloped roof, poorly ventilation
Transparent PV » Facade, semi-ventilated
Facade integrated » Facade, poorly ventilated
On sloped roof » Sloped roof, non-ventilated
I hesitated on removing the transparent
part of the name, but the author seems to point out more to a possible interior airflow rather than the transparency. Maybe we can add a note referring that the two facades also differed in the materials/transparency of the module structure.
Would also include a note mentioning that the ventilation in the namings refers to the back side of the module (and, thus, the spacing available with the surface).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, ventilation is the primary distinction, rather than level of transparency of the module itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwhanse documentation improved in latest commit, let me know what you think. And thanks for the brainstorming! :-)
docs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for 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.I think the issue is clear enough.