Skip to content
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

PVSystem.temperature_model_parameters requirement #1030

Closed
wholmgren opened this issue Aug 25, 2020 · 6 comments · Fixed by #1033
Closed

PVSystem.temperature_model_parameters requirement #1030

wholmgren opened this issue Aug 25, 2020 · 6 comments · Fixed by #1033
Milestone

Comments

@wholmgren
Copy link
Member

The temperature_model_parameters handling code below suggests to me that in 0.8 we're going to

  1. set default values module_type=None and racking_model=None.
  2. require user to specify either temperature_model_parameters or both module_type and racking_model.

# TODO: deprecated behavior if PVSystem.temperature_model_parameters
# are not specified. Remove in v0.8
if not any(self.temperature_model_parameters):
warnings.warn(
'Required temperature_model_parameters is not specified '
'and parameters are not inferred from racking_model and '
'module_type. Reverting to deprecated default: SAPM cell '
'temperature model parameters for a glass/glass module in '
'open racking. In the future '
'PVSystem.temperature_model_parameters will be required',
pvlibDeprecationWarning)
params = temperature._temperature_model_params(
'sapm', 'open_rack_glass_glass')
self.temperature_model_parameters = params

@cwhanse is that correct?

The problem is that the only way to see this warning is to supply an invalid module_type or racking_model. That's because PVSystem._infer_temperature_model is called before the code above, and it looks up the default module_type and racking_model and successfully finds temperature coefficients.

if temperature_model_parameters is None:
self.temperature_model_parameters = \
self._infer_temperature_model_params()

So I'm guessing that this warning has been seen by only a small fraction of people that need to see it. I'm ok moving forward with the removal in 0.8 or pushing to 0.9.

@wholmgren wholmgren added this to the 0.8.0 milestone Aug 25, 2020
@cwhanse
Copy link
Member

cwhanse commented Aug 25, 2020

The temperature_model_parameters handling code below suggests to me that in 0.8 we're going to

  1. set default values module_type=None and racking_model=None.
  2. require user to specify either temperature_model_parameters or both module_type and racking_model.

@cwhanse is that correct?

Yes, that is the intent.

So I'm guessing that this warning has been seen by only a small fraction of people that need to see it. I'm ok moving forward with the removal in 0.8 or pushing to 0.9.

The warning should have been raised whenever condition #2 above wasn't met; it looks to me that has been the case. If that hasn't been the case I would prefer to fix the warning and push the deprecation out to v0.9. pvlib-python has had that unadvertised default temperature model assignment for a long time.

@cwhanse
Copy link
Member

cwhanse commented Aug 25, 2020

The problem is that the only way to see this warning is to supply an invalid module_type or racking_model. That's because PVSystem._infer_temperature_model is called before the code above, and it looks up the default module_type and racking_model and successfully finds temperature coefficients.

I don't follow here - it looks to me that the warning should be raised if 1) temperature_model_parameters isn't specified, or 2) either module_type or racking_model are invalid. Maybe we're saying the same thing. _infer_temperature_model doesn't assign the default temperature model, that is done in the block of code that raises the warning.

@wholmgren
Copy link
Member Author

The module_type and racking_model defaults prevent the warning from showing up in many use cases. If we change the defaults to None then the warning will be triggered. If we simultaneously remove the warning then code will break without users having ever seen the warning.

@wholmgren
Copy link
Member Author

What is the expected behavior for PVSystem()?

@cwhanse
Copy link
Member

cwhanse commented Aug 25, 2020

The module_type and racking_model defaults prevent the warning from showing up in many use cases. If we change the defaults to None then the warning will be triggered. If we simultaneously remove the warning then code will break without users having ever seen the warning.

Aha. I see the problem now, thanks. Perhaps remove the defaults in v0.8 and leave the warning until v0.9?

@wholmgren
Copy link
Member Author

Ok, I'll work on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants