-
Notifications
You must be signed in to change notification settings - Fork 1.1k
remove 0.8 deprecations from pvsystem.py, modelchain.py, rework temperature model param deprecation #1033
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
Moving the deprecation warning from def infer_temperature_model(self):
params = set(self.system.temperature_model_parameters.keys())
# revert this change in 0.9
if set(['a', 'b', 'deltaT']) <= params or (if not params and self.system.racking_model is None and self.system.module_type is None):
return self.sapm_temp # system.sapm_celltemp will assign default keys and emit warning at runtime Does that all seem reasonable @cwhanse? |
pvlib/pvsystem.py
Outdated
'in open racking. In v0.9 ' | ||
'PVSystem.temperature_model_parameters will be empty dict ' | ||
'unless set by temperature_model_parameters or valid ' | ||
'combination of racking_model and module_type. Pass non-None ' |
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.
'in open racking. In v0.9 ' | |
'PVSystem.temperature_model_parameters will be empty dict ' | |
'unless set by temperature_model_parameters or valid ' | |
'combination of racking_model and module_type. Pass non-None ' | |
'in open racking. In v0.9, temperature_model_parameters or a valid ' | |
'combination of racking_model and module_type will be required. ' | |
' Pass non-None ' |
I'd prefer to be more specific how to use v0.9.
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 "will be required" if the warning is emitted from PVSystem.sapm_celltemp
. I don't like it if the warning is emitted from the PVSystem.__init__
because it suggests a positional argument.
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.
PVSystem()
has always been a valid, non-warned statement. There's never been a requirement thatPVSystem
is a complete description of a system according to some model. We can discuss changing that, but it's inconsistent to require temperature parameters and nothing else.
Agree, and point taken. I'm OK with the change proposed for ModelChain.infer_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.
changed the text in the warning in the method
I'm not following here - why move the warning from |
|
I think this is now ready to merge. Sorry for the churn as I worked out inconsistencies in flake8 settings between stickler and running locally. Added a flake8 section to The |
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
FYI I noticed there's still an entry to |
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`
).I'm still struggling with the right approach for #1030. Here are examples of the behavior in this PR:
This change also causes 28 new warnings from just
test_pvsystem.py
. I'm sure there are more intest_modelchain.py
. In practice, I think this only matters to users that callPVSystem.sapm_celltemp
, so we could push the deprecation down to this method.