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

Fix error message in ModelChain pertaining to temp_model inference #1946

Closed
matsuobasho opened this issue Jan 11, 2024 · 8 comments · Fixed by #1977
Closed

Fix error message in ModelChain pertaining to temp_model inference #1946

matsuobasho opened this issue Jan 11, 2024 · 8 comments · Fixed by #1977

Comments

@matsuobasho
Copy link
Contributor

Describe the bug
Error message is incorrect.
The message is:

ValueError: could not infer temperature model from system.temperature_model_parameters. Check that all Arrays in system.arrays have parameters for the same temperature model. Common temperature model parameters: set().

However, in code below we are creating a PVSystem without using Arrays. So the second sentence in the message above is not describing the proper way to fix this issue.

To Reproduce

from pvlib import pvsystem
from pvlib.location import Location
from pvlib.modelchain import ModelChain

location = Location(latitude=32.2, longitude=-110.9)

modules = pvsystem.retrieve_sam('cecmod')
module_parameters = modules['Canadian_Solar_Inc__CS5P_220M']
inverter_parameters = {'pdc0': 5000, 'eta_inv_nom': 0.96}
system = pvsystem.PVSystem(inverter_parameters=inverter_parameters,
                           module_parameters=module_parameters)

# create the model chain
modelchain = ModelChain(
    system, location, name='dummy_name', aoi_model="physical", temperature_model="pvsyst"
)

Expected behavior
Error message correctly explains what needs to be changed in pvsystem insantiation in order for ModelChain to run.

Versions:

  • pvlib.__version__: 0.10.2
  • python: 3.9.6

Additional context
This error message also appears in other contexts. Namely for other model types:
AC
AOI
DC
spectral

So the error messages for those models should be fixed as well. Search for 'could not infer' to see which messages.

@matsuobasho
Copy link
Contributor Author

matsuobasho commented Feb 6, 2024

I started to tackle this issue. 3 questions:

  1. Following @cwhanse wording for the updated error message:
    "Could not infer the temperature model. If Arrays are not used to construct the PVsystem, check the PVsystem attributes racking_model and module_type. If Arrays are used, check that all Arrays in ModelChain.system.arrays have parameters for the same temperature model. Common temperature model parameters: {params}."

I did some experimentation when specifying 2 arrays for the PVSystem:

location = Location(latitude=32.2, longitude=-110.9)

modules = pvsystem.retrieve_sam('cecmod')
module_parameters = modules['Canadian_Solar_Inc__CS5P_220M']
inverter_parameters = {'pdc0': 5000, 'eta_inv_nom': 0.96}

module_parameters = {'pdc0': 5000, 'gamma_pdc': -0.004}

mount = pvsystem.FixedMount(surface_tilt=20, surface_azimuth=180)

array_one = pvsystem.Array(mount=mount, module_parameters=module_parameters, temperature_model_parameters={'a': -3.47, 'b': -.0594, 'deltaT': 3})
array_two = pvsystem.Array(mount=mount, module_parameters=module_parameters, temperature_model_parameters={'a': -3.56, 'b': -.0750, 'deltaT': 3}
system_two_arrays = pvsystem.PVSystem(arrays=[array_one, array_two],
                                      inverter_parameters=inverter_parameters)

modelchain = ModelChain(
    system_two_arrays, location, name='dummy_name', aoi_model="physical", temperature_model="sapm", spectral_model="no_loss"
)

I'm not following the reasoning of the last sentence in the error message. If we specify any dictionary from the same temp model family as above, then we don't get the error at all.

TEMPERATURE_MODEL_PARAMETERS = {
    'sapm': {
        'open_rack_glass_glass': {'a': -3.47, 'b': -.0594, 'deltaT': 3},
        'close_mount_glass_glass': {'a': -2.98, 'b': -.0471, 'deltaT': 1},
        'open_rack_glass_polymer': {'a': -3.56, 'b': -.0750, 'deltaT': 3},
        'insulated_back_glass_polymer': {'a': -2.81, 'b': -.0455, 'deltaT': 0},
    },
    'pvsyst': {'freestanding': {'u_c': 29.0, 'u_v': 0},
               'insulated': {'u_c': 15.0, 'u_v': 0}}
}

However, if we specify one array to be from 'sapm' and the other one from 'pvsyst' then the error reads:
"Common temperature model parameters: set()."
which is confusing to a user.

  1. Is the wording of the other similar error messages in modelchain that pertain to AC, AOI, DC, spectral in scope to be updated for this issue? For example, here.

  2. Detailed explanation of racking_module and module_type in PVSystem docstring #1942 is a related issue that might be worthwhile tackling first (though I don't have knowledge to take it on without help). That way we can directly point the user to where to read about racking_module and module_type in the updated error message. Open to guidance but happy to just focus on this issue first as it is more straightforward.

@matsuobasho
Copy link
Contributor Author

@cwhanse or one of the other maintainers, can you please provide some pointers on the above. I would like to start working to resolve this issue. Thanks.

@cwhanse
Copy link
Member

cwhanse commented Feb 15, 2024

@matsuobasho pvlib's ModelChain currently only supports a single temperature model for all arrays. There's no way to set a temperature model for each array. We don't need to test or provide an error message if a user specifies temperature model parameters from different models, for different arrays.

We should allow for a user to specify different parameters for the same temperature model, as is done in your example with SAPM. When I run that code, I don't get the error, even after adding a modelchain.run_model call. I think that is correct behavior.

Re-reading that error message text, I would edit it as:

"Could not infer the temperature model. If Arrays are used to construct the PVSystem, check that all Arrays in ModelChain.system.arrays have parameters for the same temperature model. If Arrays are not used, check that the PVsystem attributes racking_model and module_type are valid."

I'm now on the fence if returning " Common temperature model parameters: {params}." is useful.

@matsuobasho
Copy link
Contributor Author

Regarding your first point, the following comment really pertains to #1942 - the assumption is that the user understands that different parameters in the format
{'a': -3.47, 'b': -.0594, 'deltaT': 3} are from the same temperature model (i.e. there are only 2 temp models). I think a new user may incorrectly consider 'open_rack_glass_glass', 'close_mount_glass_glass', etc to be different temperature models.

I'll incorporate the wording you propose.

Can you please make a recommendation about whether the wording of the other messages in modelchain that pertain to AC, AOI, DC, spectral are in scope in this issue?

@cwhanse
Copy link
Member

cwhanse commented Feb 21, 2024

Can you please make a recommendation about whether the wording of the other messages in modelchain that pertain to AC, AOI, DC, spectral are in scope in this issue?

Let's address only the message for the temperature model this issue. When we get concurrence on that change, we can make similar changes to the other messages.

@matsuobasho
Copy link
Contributor Author

I submitted the PR 1 week ago, requesting feedback on it. Thank you.

@cwhanse
Copy link
Member

cwhanse commented Mar 1, 2024

@matsuobasho can you fix the linter issues? I could push to your fork if you would like help.

@matsuobasho
Copy link
Contributor Author

Thanks @cwhanse should have seen that fail in the build. Pushed the update now, please let me know if there's anything else.

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