-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Expose temperature.fuentes in PVSystem and ModelChain #1073
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
""" | ||
kwargs = _build_kwargs([ | ||
'noct_installed', 'module_height', 'wind_height', 'emissivity', | ||
'absorption', 'surface_tilt', 'module_width', 'module_length'], |
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 think 'surface_tilt'
is a PVSystem
attribute, not to be found in temperature_model_parameters
We might also consider getting 'module_width'
and 'module_length'
from module_parameters
, although: the database doesn't supply these values and they aren't used anywhere else. 'module_height'
is really an attribute of the PV System.
For now, I'm OK expecting the 'module_'
values in temperature_model_parameters
but they will move once another, non-temperature function needs this information - e.g., a bifacial irradiance function.
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.
Valid points. I unthinkingly pulled all the extra parameters from temperature.fuentes
and dumped them there.
I can imagine some people objecting to using Fuentes with the real surface_tilt
value because they want the same behavior as the SSC implementation of PVWatts, which hardcodes the tilt at 30. Would it make sense to use the PVSystem
attributes by default but allow the user to override them by providing alternate values in the temperature_model_parameters
dictionary?
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 have a better idea at the moment, than using temperature_model_parameters['surface_tilt']
as the signal to override. But I think it risks confusing users, to have two 'surface_tilt' quantities with different meanings. Maybe a Note in the PVSystem.celltemp_fuentes
docstring and move ahead.
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.
Also something to consider when creating an Array class.
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
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.
trivial doc fix @kanderso-nrel
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
docs/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`
).Basically just copied #897. Happy to make changes if needed. CI is probably going to complain about this until #1071 is fixed.