Resolving AttributeError when using ModelChain with multiple Arrays #1938

matsuobasho · 2023-12-22T19:03:10Z

Closes AttributeError when using ModelChain with multiple Arrays #1759
I am familiar with the contributing guidelines
Tests added
Updates entries in docs/sphinx/source/reference for API changes. -- Doesn't seem to be relevant for PVSystem
Adds description and name entries in the appropriate "what's new" file in 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`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

…attribute-error-mult-arrays

…till None after attempt to infer

…heck

… inference to be more informative to user

…uired args to infer temperature_model_parameters

…nference

…tive for user to diagnose

…el parameters

…reflect new required argument

cwhanse

Formatting suggestions

cwhanse · 2024-01-01T19:17:34Z

docs/sphinx/source/whatsnew/v0.10.4.rst


 Testing
 ~~~~~~~


 Documentation
 ~~~~~~~~~~~~~
-
+* Update all examples in :ref:`pvsystem` User's guide page that reference
+`pvsystem` to adhere to new required argument.


Suggested change

`pvsystem` to adhere to new required argument.

`pvsystem` to adhere to new required argument.

cwhanse · 2024-01-01T19:17:51Z

docs/sphinx/source/whatsnew/v0.10.4.rst

+error in `ModelChain` instantiation when the `temperature_model_parameters`
+for a `PVSystem` have not been set.  Update requires `PVSystem` or `Array`
+to have `temperature_model_parameters` explicitly or implicitly by providing
+`racking_model` and `module_type`.


Suggested change

error in `ModelChain` instantiation when the `temperature_model_parameters`

for a `PVSystem` have not been set. Update requires `PVSystem` or `Array`

to have `temperature_model_parameters` explicitly or implicitly by providing

`racking_model` and `module_type`.

error in `ModelChain` instantiation when the `temperature_model_parameters`

for a `PVSystem` have not been set. Update requires `PVSystem` or `Array`

to have `temperature_model_parameters` explicitly or implicitly by providing

`racking_model` and `module_type`.

cwhanse · 2024-01-01T19:19:22Z

pvlib/pvsystem.py

@@ -977,6 +984,18 @@ def __init__(self, mount,
        else:
            self.temperature_model_parameters = temperature_model_parameters

+        if self.temperature_model_parameters=={}:


Suggested change

if self.temperature_model_parameters=={}:

if self.temperature_model_parameters == {}:

cwhanse · 2024-01-01T19:19:33Z

pvlib/pvsystem.py

@@ -977,6 +984,18 @@ def __init__(self, mount,
        else:
            self.temperature_model_parameters = temperature_model_parameters

+        if self.temperature_model_parameters=={}:
+            raise ValueError("The `temperature_model_parameters` is empty "
+                            "after an attempt to infer it. "


Suggested change

"after an attempt to infer it. "

"after an attempt to infer it. "

cwhanse · 2024-01-01T19:19:40Z

pvlib/pvsystem.py

+        if self.temperature_model_parameters=={}:
+            raise ValueError("The `temperature_model_parameters` is empty "
+                            "after an attempt to infer it. "
+                            "Pass either `temperature_model_parameters` to "


Suggested change

"Pass either `temperature_model_parameters` to "

"Pass either `temperature_model_parameters` to "

cwhanse · 2024-01-01T19:19:52Z

pvlib/pvsystem.py

+            raise ValueError("The `temperature_model_parameters` is empty "
+                            "after an attempt to infer it. "
+                            "Pass either `temperature_model_parameters` to "
+                            "`Array` or `PVSystem` (if not passing arrays), or "


Suggested change

"`Array` or `PVSystem` (if not passing arrays), or "

"`Array` or `PVSystem` (if not passing arrays), or"

cwhanse · 2024-01-01T19:20:22Z

pvlib/pvsystem.py

+                            "after an attempt to infer it. "
+                            "Pass either `temperature_model_parameters` to "
+                            "`Array` or `PVSystem` (if not passing arrays), or "
+                            "`racking_module` to the `Array` `mount` "


Suggested change

"`racking_module` to the `Array` `mount` "

" `racking_module` to the `Array` `mount` "

cwhanse · 2024-01-01T19:20:40Z

pvlib/pvsystem.py

+                            "Pass either `temperature_model_parameters` to "
+                            "`Array` or `PVSystem` (if not passing arrays), or "
+                            "`racking_module` to the `Array` `mount` "
+                            "object and `module_type` to `Array.  For guidance "


Suggested change

"object and `module_type` to `Array. For guidance "

"object and `module_type` to `Array. For guidance"

cwhanse · 2024-01-01T19:21:13Z

pvlib/pvsystem.py

+                            "`Array` or `PVSystem` (if not passing arrays), or "
+                            "`racking_module` to the `Array` `mount` "
+                            "object and `module_type` to `Array.  For guidance "
+                            "on allowed values to use for `racking_module` and "


Suggested change

"on allowed values to use for `racking_module` and "

" on values allowed for `racking_module` and "

cwhanse · 2024-01-01T19:23:17Z

pvlib/tests/test_pvsystem.py

+    mount = pvsystem.FixedMount(surface_tilt=20, surface_azimuth=180,
+                                racking_model=racking_model)
+    with pytest.raises(ValueError, match='`temperature_model_parameters` is empty'):
+        pvsystem.Array(mount = mount, module_type=module_type)


Suggested change

pvsystem.Array(mount = mount, module_type=module_type)

pvsystem.Array(mount=mount, module_type=module_type)

matsuobasho · 2024-01-03T22:57:24Z

@cwhanse, these all look like formatting changes that I can accept right here. However, would like to understand the reason for these updates. For example:

Why do we need the line to begin with a space?

Is there a setting that I need to tweak in my linter / yapf to format it according to this standard? The only reference to spaces I see in the Contributing page is

Set your editor to strip extra whitespace from line endings. This prevents the git commit history from becoming cluttered with whitespace changes.

Thanks

… document

matsuobasho · 2024-01-03T23:18:44Z

I noticed that the Contributing document has 2 instances of PVSystem instantiation, which are just defined but not executed, so no error would come up during the build. Added the new required temperature_model_parameters argument in the 2 places of instantiation.

cwhanse · 2024-01-04T00:58:08Z

Why do we need the line to begin with a space?

It may be unnecessary but it makes the text file easier to read.

cwhanse · 2024-01-04T01:06:31Z

@pvlib/pvlib-maintainer this PR will change default behavior, although the current behavior is buggy. It will be required to provide either temperature_model_parameters, or a proper combination of racking_model and module_type. In the current behavior, a PVSystem can be instantiated without these inputs. In this PR, a ValueError would be raised.

What do we think about deprecation for this change? I view the change as primarily correcting a bug in v0.9.0 where we dropped defaulting to a SAPM temperature model when the inputs weren't supplied.

kandersolar · 2024-01-04T13:45:04Z

Why do we need the line to begin with a space?

It may be unnecessary but it makes the text file easier to read.

Those RST files are used to build our documentation, and the spaces are necessary for that multi-line text to be considered a single bullet item instead of a bullet followed by a new paragraph. That file isn't included in our docs build yet, but you can check that currently it is not rendering correctly using GitHub's viewer: https://github.com/matsuobasho/pvlib-python/blob/main/docs/sphinx/source/whatsnew/v0.10.4.rst

In the current behavior, a PVSystem can be instantiated without these inputs. In this PR, a ValueError would be raised.

I still need to read and digest the discussion in #1759, but on the face of it, this seems not ideal to me. Currently it's possible to instantiate a PVSystem with no arguments at all. Seems strange to me for these temperature model inputs to be the only required inputs.

matsuobasho · 2024-01-05T17:02:05Z

@cwhanse please confirm my interpretation that it's best to wait without making any updates to this until further word

cwhanse · 2024-01-05T17:15:35Z

@matsuobasho yes, we're sorting out what the desired behavior should be.

cwhanse · 2024-01-11T16:24:55Z

pvlib/pvsystem.py

@@ -977,6 +984,18 @@ def __init__(self, mount,
        else:
            self.temperature_model_parameters = temperature_model_parameters

+        if self.temperature_model_parameters=={}:


@matsuobasho I agree with @kandersolar point of view, that we should remove this ValueError. I think we'll want to undo most of the changes in test_pvsystem.py as well. Because I can push to your fork, let me know if you'd like some help. And thanks again for your effort here.

It'll be most efficient to delete this entire MR because there are so many changes that will not be used. The proposed change is just deleting two lines and will be easiest to recreate with a new MR.

Let me know if you can kill the entire MR. I'll then create a new MR with just that one change.

In this case, it looks like we do not need a new test or any docstring modifications?

You can close this PR and start a new one.

For the new PR I recommend that you first create a branch in your fork and use that branch for changes. Its not usually a good idea to make changes in your main branch.

First reset your main branch

git reset --hard upstream/ main where upstream is your name for pvlib/pvlib-python

Then create a branch

git checkout -b

Make the edits in the branch, add, commit, push, etc.

matsuobasho added 15 commits December 14, 2023 20:01

Delete racking_model check in infer_temperature_model of model_chain

37ddf59

Merge branch 'main' of github.com:matsuobasho/pvlib-python into 1759-…

dde73ee

…attribute-error-mult-arrays

In pvsystem __init__ raise error if temperarure_model_parameters is s…

d1b3d80

…till None after attempt to infer

Grammatical typo correction in ValueError in null temp_model_params c…

fa0c606

…heck

Merge branch 'pvlib:main' into main

8bcbaa7

Updated error message when temperature_model_parameters is null after…

2f4ada3

… inference to be more informative to user

Add test for raising value error when instantiating array without req…

13a5bc4

…uired args to infer temperature_model_parameters

Correct if condition in checking temperature_model_parameters after i…

8107130

…nference

Added some additional details to the error message to make it informa…

2bafd68

…tive for user to diagnose

Add raises section in docstring of Array to describe error

5f2abdf

Update pvsystem user guide to reflect requirement for temperature mod…

72bf8f0

…el parameters

Update instances of pvsystem instantiation in test_pvsystem tests to …

196be6a

…reflect new required argument

Add temp_model_params fixture to test_pvsystem.py

013b1a2

Merge branch 'pvlib:main' into main

fd5eb63

Update what's new

fe40f1b

cwhanse reviewed Jan 1, 2024

View reviewed changes

Added temp_model_parameters to pvsystem instantiation in Contributing…

2e02c49

… document

cwhanse reviewed Jan 11, 2024

View reviewed changes

matsuobasho closed this Jan 11, 2024

cwhanse mentioned this pull request Jan 18, 2024

1759 resolve attrib error model chain #1947

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolving AttributeError when using ModelChain with multiple Arrays #1938

Resolving AttributeError when using ModelChain with multiple Arrays #1938

matsuobasho commented Dec 22, 2023

cwhanse left a comment

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

cwhanse Jan 1, 2024

matsuobasho commented Jan 3, 2024

matsuobasho commented Jan 3, 2024

cwhanse commented Jan 4, 2024

cwhanse commented Jan 4, 2024

kandersolar commented Jan 4, 2024

matsuobasho commented Jan 5, 2024

cwhanse commented Jan 5, 2024

cwhanse Jan 11, 2024

matsuobasho Jan 11, 2024

cwhanse Jan 11, 2024

	`pvsystem` to adhere to new required argument.
	`pvsystem` to adhere to new required argument.

	if self.temperature_model_parameters=={}:
	if self.temperature_model_parameters == {}:

	"after an attempt to infer it. "
	"after an attempt to infer it. "

	"Pass either `temperature_model_parameters` to "
	"Pass either `temperature_model_parameters` to "

	"`Array` or `PVSystem` (if not passing arrays), or "
	"`Array` or `PVSystem` (if not passing arrays), or"

	"`racking_module` to the `Array` `mount` "
	" `racking_module` to the `Array` `mount` "

	"object and `module_type` to `Array. For guidance "
	"object and `module_type` to `Array. For guidance"

	"on allowed values to use for `racking_module` and "
	" on values allowed for `racking_module` and "

	pvsystem.Array(mount = mount, module_type=module_type)
	pvsystem.Array(mount=mount, module_type=module_type)

Resolving AttributeError when using ModelChain with multiple Arrays #1938

Resolving AttributeError when using ModelChain with multiple Arrays #1938

Conversation

matsuobasho commented Dec 22, 2023

cwhanse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsuobasho commented Jan 3, 2024

matsuobasho commented Jan 3, 2024

cwhanse commented Jan 4, 2024

cwhanse commented Jan 4, 2024

kandersolar commented Jan 4, 2024

matsuobasho commented Jan 5, 2024

cwhanse commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment