Skip to content

1759 resolve attrib error model chain #1947

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 23, 2024
Merged

1759 resolve attrib error model chain #1947

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6238e51
Remove or clause from if clause in infer_temperature_model in ModelChain
matsuobasho Jan 11, 2024
821d1fc
Updated what's new
matsuobasho Jan 11, 2024
17e6ee2
Remove comment about removing or statement in infer_temperature_model…
matsuobasho Jan 19, 2024
5cde61f
Added test to verify that error indicating inability to infer tempera…
matsuobasho Jan 19, 2024
00c0b8b
Update docs/sphinx/source/whatsnew/v0.10.4.rst
matsuobasho Jan 19, 2024
022eabe
Update pvlib/tests/test_modelchain.py
kandersolar Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/sphinx/source/whatsnew/v0.10.4.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ Enhancements

Bug fixes
~~~~~~~~~

* Removed `or` clause from `if` statement in `infer_temperature_model` in
`ModelChain` (:issue:`1759`).

Testing
~~~~~~~
Expand All @@ -27,4 +28,4 @@ Requirements

Contributors
~~~~~~~~~~~~

* :ghuser:`matsuobasho`
4 changes: 1 addition & 3 deletions pvlib/modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,9 +1118,7 @@ def infer_temperature_model(self):
array.temperature_model_parameters for array in self.system.arrays)
params = _common_keys(temperature_model_parameters)
# remove or statement in v0.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's delete this comment as well, since we are now removing the specified or

Copy link
Contributor Author

@matsuobasho matsuobasho Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do about removing the or statement, nice catch.

Thanks for creating the test. I have 2 comments about it before implementing. The PVSystem docstring states:

If `arrays` is
        specified the following PVSystem parameters are ignored:

Having stepped away from this and looking at the error I realized that understanding why the temperature model parameter was ignored requires the user to dig through that long PVsystem docstring.

I think it's worthwhile to update the error message to make it easier on the user:

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.  Note that if arrays are passed, many system arguments are ignored. Common temperature model parameters: set().

Also, we need to update that last sentence in the error message, it's confusing. Not sure where this set() is coming from, looking for guidance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that passing temperature_model_parameters alongside arrays is not a valid use for PVSystem. If a user did this, they should not expect it to work. But the intent of this test is to make sure that, when this incorrect usage occurs, the correct error is raised: ValueError: could not infer temperature... instead of AttributeError: 'PVSystem' object has no attribute 'racking_model'. What you suggest (do not specify temperature_model_parameters at all) would be a verification of some other behavior, not that the issue from #1759 is resolved.

If you like, please clarify the explanation comment in the test for future readers :)

Copy link
Contributor Author

@matsuobasho matsuobasho Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was updating my comment while you were replying. I'm not proposing modifying the test by not specifying temperature_model_parameters. I'm proposing updating the error message to make it more informative, as well as fixing the last sentence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating that error message is the issue in #1946, right? I think we should fix one thing at a time, and this PR should focus just on #1759.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this ValueError, what about

"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}."

The params dict is what ModelChain found when it tries to infer the temperature model, and is provided here to (hopefully) help the user to diagnose and fix the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kandersolar, you are correct, makes sense, thanks. Will incorporate as per your suggestion and then after we finish probably tackle #1946 to address the error message. Thanks @cwhanse for the phrasing.

if {'a', 'b', 'deltaT'} <= params or (
not params and self.system.racking_model is None
and self.system.module_type is None):
if {'a', 'b', 'deltaT'} <= params:
return self.sapm_temp
elif {'u_c', 'u_v'} <= params:
return self.pvsyst_temp
Expand Down