-
Notifications
You must be signed in to change notification settings - Fork 1.1k
NOCT cell temperature function #1177
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
Conversation
Ready for review. check failure is codecov, but all 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.
Mostly looks good. Are you planning a follow up issue/pr for integration into PVSystem
and ModelChain
?
pvlib/tests/test_temperature.py
Outdated
expected = 54.41542289 | ||
result = temperature.noct(poa_global, temp_air, wind_speed, noct, | ||
eta_m_ref) | ||
assert np.isclose(result, expected) |
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.
assert np.isclose(result, expected) | |
assert_allclose(result, expected) |
pvlib/tests/test_temperature.py
Outdated
result = temperature.noct(np.array(poa_global), np.array(temp_air), | ||
np.array(wind_speed), np.array(noct), | ||
np.array(eta_m_ref)) | ||
assert np.isclose(result, expected) |
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.
assert np.isclose(result, expected) | |
assert_allclose(result, expected) |
pvlib/tests/test_temperature.py
Outdated
transmittance_absorbtance, array_height, | ||
mount_standoff) | ||
expected = 58.36654459 | ||
assert np.isclose(result, expected) |
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.
assert np.isclose(result, expected) | |
assert_allclose(result, expected) |
pvlib/temperature.py
Outdated
''' | ||
Cell temperature model from the System Advisor Model (SAM). | ||
|
||
The model is described in [1], Section 10.6. |
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 model is described in [1], Section 10.6. | |
The model is described in [1]_, Section 10.6. |
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.
SAM document says "The NOCT photovoltaic cell temperature model is from Chapter 23.3 of Duffie and Beckman (2013), also de- scribed in De Soto (2004b)." Should we mention those references as well?
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 with the comment. Let me see if I can locate a copy of Duffie and Beckman. There's an issue with the De Soto reference, I downloaded both (paper and thesis) and there's no discussion in either of the cell temperature model.
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.
Maybe the reference to De Soto is for the reflection model? https://github.com/NREL/ssc/blob/055e2847ea1f4a81b4a0062beb972ec45e77d185/shared/lib_cec6par.cpp#L176-L182
pvlib/temperature.py
Outdated
@@ -706,3 +706,94 @@ def fuentes(poa_global, temp_air, wind_speed, noct_installed, module_height=5, | |||
sun0 = sun | |||
|
|||
return pd.Series(tmod_array - 273.15, index=poa_global.index, name='tmod') | |||
|
|||
|
|||
def _adj_noct(x): |
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.
something more descriptive like _adj_for_mounting_standoff
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.
SAM document is not clear on if we should use >=
or <=
. Maybe check the ssc
code for consistency? And probably add a comment on this detail, too.
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.
SAM code doesn't process a distance, only passes through an integer index to the values. Except for the "x > 3.5" case, the SAM interface text doesn't specify > or >=. So we're filling in a detail here. I'll adjust the default to 3.51 and put in some comments.
pvlib/temperature.py
Outdated
cell_temp_init = ross(poa_global, temp_air, noct_adj) | ||
heat_loss = 1 - eta_m_ref / tau_alpha | ||
wind_loss = 9.5 / (5.7 + 3.8 * wind_adj) | ||
return cell_temp_init * heat_loss * wind_loss |
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 don't think this is correct. ross
returns air temperature + temperature correction. You want to multiply only temperature correction by heat_loss
and wind_loss
, but you've also multiplied the air temperature by these terms
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.
You are correct. We can't re-use ross
here.
Yes, I wanted to get consensus on the function first. |
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'm only partially able to reconcile the standoff adjustment values in SAM Tech Ref Eq. 10.36 with their supposed source, Fuentes Table 4. Not sure where some of these SAM/SSC values came from (e.g. 2inch -> +6C?); it almost seems like they interpolated/projected the Fuentes values. Maybe that makes sense here too -- to go from discrete to continuous, just interpolate instead of bin? Although that would be more of a departure from SAM/SSC.
Some helpful links for anyone following along:
- SAM technical reference: https://www.nrel.gov/docs/fy18osti/67399.pdf
- Fuentes: https://prod-ng.sandia.gov/techlib-noauth/access-control.cgi/1985/850330.pdf
- SSC implementation: https://github.com/NREL/ssc/blob/055e2847ea1f4a81b4a0062beb972ec45e77d185/shared/lib_cec6par.cpp#L160
- Additional SSC settings: https://github.com/NREL/ssc/blob/055e2847ea1f4a81b4a0062beb972ec45e77d185/shared/lib_pv_io_manager.cpp#L957-L963
tau_alpha = transmittance_absorbtance * irr_ratio | ||
|
||
# [1] Eq. 10.37 isn't clear on exactly what "G" is. SAM SSC code uses | ||
# poa_global where G appears |
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.
At the bottom of page 57 it says "The cell temperature is only calculated when the effective transmitted irradiance from Equation 10.25 is greater than zero, G>0.", and tracing back through to Eq 10.20 shows that G
is after accounting for surface reflection and airmass.
if effective_irradiance is None: | ||
irr_ratio = 1. | ||
else: | ||
irr_ratio = effective_irradiance / 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.
Not sure this is correct. Eq 10.22 has the ratio as G0/G, where G0 is irradiance after reflections and G is after reflections and airmass.
I think we should regard SAM as the source for standoff adjustment values. About "G" vs. "G0" in the SAM document: the term "G" is redefined twice in Chapter 10 (Eq. 10.13 and 10.22 are POA broadband, Eq. 10.25 and 10.26 (and 7.8) are effective irradiance absorbed by the cells. I don't read C well, but it looked to me that It would be desirable if this temperature model could be used without requiring both The SAM implementation is more involved that what is described in Duffie and Beckman. One way forward would be to implement
|
I spent some time trying to get SAM to give me back cell temperatures, without success. That quantity isn't available in the time series output, and the relevant function ( At this point, my recommendation is to fall back to implementing the model in Duffie and Beckman, and adding the SAM model in the future (as a different function) when we're clear exactly what the SAM model does. @kanderso-nrel? @wholmgren? The driver for this PR is the Solar Performance Insight project, where it would be nice to have a pre-packaged "SAM" model chain. |
No preference from me on the way best way forward. For getting cell temps from SAM: it is available in the UI, but maybe not under a name you expect (Subarray 1 Cell Temperature), see screenshots below. You can grab numerical values from the |
@kanderso-nrel thanks for those pointers to SAM output. I am able to reproduce the Subarray 1 Cell Temperature values. To reproduce, I equated the following terms used in the SAM code with pvlib variables:
I will put comments in the pvlib code to clarify why we assign these variables and write one test that reproduces the SAM value. |
Failed check is codecov. This is ready to go, I think. |
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 test_noct_sam_against_sam
. I don't know what's going on with codecov. Maybe we need to start specifying a target value for that configuration.
pvlib/temperature.py
Outdated
temp_air : numeric | ||
Ambient dry bulb temperature. [C] | ||
|
||
wind_speed : numeric, default 1.0 |
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.
no default value in the function signature
pvlib/temperature.py
Outdated
The irradiance that is converted to photocurrent. If None, | ||
assumed equal to poa_global. [W/m^2] | ||
|
||
eta_m_ref : float |
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.
comes before effective_irradiance in function signature.
Can we call this module_efficiency
so that more people can easily understand it?
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'm ok with module_efficiency
but I'd rather have a common name with pvsyst_cell
, where eta_m
is the same quantity. I used eta_m_ref
here because for SAM it's clear that it's at STC, which is not clear for Pvsyst.
pvlib/temperature.py
Outdated
20C. Calculate as | ||
.. math:: | ||
|
||
\eta_{m} = \frac{V_{mp} I_{mp}}{A \times 1000 W/m^2} # noQA: W605 |
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'm surprised you need noqa: W605 since the whole docstring is preceded by r
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.
Suggest changing this to inline with :math:
as right now it's rendering as a code block (not a math block) on RTD.
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 had the noQA
before I put the r
in the docstring. Should be OK without the noQA
now.
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.
After taking a closer look and correcting my understanding of the SSC implementation, LGTM besides @wholmgren's comments and a formatting note below.
pvlib/temperature.py
Outdated
20C. Calculate as | ||
.. math:: | ||
|
||
\eta_{m} = \frac{V_{mp} I_{mp}}{A \times 1000 W/m^2} # noQA: W605 |
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.
Suggest changing this to inline with :math:
as right now it's rendering as a code block (not a math block) on RTD.
[ ] Closes #xxxxdocs/sphinx/source/api.rst
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`
).Implement cell temperature function that is the default in SAM when the CEC module model is chosen.