Deprecate/raise error for PVSystem "pass-through" properties #1196

kandersolar · 2021-03-13T19:42:45Z

~~Closes #xxxx~~
I am familiar with the contributing guidelines
Tests added
Updates entries to docs/sphinx/source/api.rst for API changes.
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 and Milestone are assigned to the Pull Request and linked Issue.

Trying out the idea suggested in #1176 (comment): for the values that used to be PVSystem attributes but have been moved down to Array, emit a deprecation warning for systems with only one Array and an error if more than one.

Note to reviewers: I was guided more by test failures than by a solid understanding of the emerging PVSystem+Array architecture. Wouldn't be surprised if I've missed something here.

kandersolar · 2021-03-28T22:00:06Z

I think this is ready for review. It feels risky to me to use system.arrays[0] in places where we expect, but don't verify, that the system has only one Array. Maybe worth adding a PVSystem.array convenience property that returns self.arrays[0] if len(self.arrays) == 1 and raises an error otherwise?

wholmgren · 2021-03-29T21:17:12Z

It feels risky to me to use system.arrays[0] in places where we expect, but don't verify, that the system has only one Array. Maybe worth adding a PVSystem.array convenience property that returns self.arrays[0] if len(self.arrays) == 1 and raises an error otherwise?

Can you elaborate? The tests pass and the diff is straightforward. We also have the property PVSystem.num_arrays. I'm open to to adding a property like PVSystem._has_single_array, but maybe that is a case of too much indirection.

I'm +1 on this PR. Would be good to hear any comments/concerns from others before moving forward.

kandersolar · 2021-03-30T00:03:11Z

It feels risky to me to use system.arrays[0] in places where we expect, but don't verify, that the system has only one Array. Maybe worth adding a PVSystem.array convenience property that returns self.arrays[0] if len(self.arrays) == 1 and raises an error otherwise?

Can you elaborate? The tests pass and the diff is straightforward. We also have the property PVSystem.num_arrays. I'm open to to adding a property like PVSystem._has_single_array, but maybe that is a case of too much indirection.

Yes, sorry for not being clear. My concern is that code like this, which does not check that len(self.arrays) == 1, puts the onus on the caller to verify that it is appropriate to call that method. Right now that contract is fulfilled, but I could see myself missing that requirement in some future refactor and accidentally introducing a subtle bug. I'd encourage us to put assumption checks like if self.num_arrays == 1 as close as possible to the code that makes the assumption. My PVSystem.array property suggestion in the previous comment is in the spirit of pandas's item(), which is a convenient way to perform the length==1 check and element retrieval in one step. If nobody else is particularly concerned about this then I'll drop it :)

pvlib/modelchain.py

cwhanse · 2021-05-06T15:32:11Z

pvlib/modelchain.py

        'desoto', 'cec', 'pvsyst', 'pvwatts'. The ModelChain instance will
        be passed as the first argument to a user-defined function.

    ac_model: None, str, or function, default None
        If None, the model will be inferred from the contents of
-        system.inverter_parameters and system.module_parameters. Valid
-        strings are 'sandia', 'adr', 'pvwatts'. The
+        system.inverter_parameters and system.arrays[i].module_parameters.


I don't see that infer_ac_model uses any information from system.arrays. The check on num_arrays is meant to provide a more meaningful error message, if a user chooses the adr inverter model that doesn't (presently) support multiple inputs.

Co-authored-by: Cliff Hansen <[email protected]>

kandersolar · 2021-05-18T16:14:11Z

It's not clear to me why codecov is flagging the diff coverage. When I calculate coverage locally it shows line 961 (really, 956) in modelchain.py covered as expected.

@wholmgren ready for you to review when you get a chance.

wholmgren · 2021-05-19T03:00:52Z

pvlib/modelchain.py

-                             'system.module_parameters or explicitly '
-                             'set the model with the dc_model kwarg.')
+                             'system.arrays[i].module_parameters. Check '
+                             'system.arrays[i].module_parameters or '


I don't know if this notation will make sense to some users. I'm fine with it, but here's an alternative:

Could not infer DC model from the module_parameters attributes of system.arrays. Check the module_parameters attributes or explicitly set the model with the dc_model keyword argument.

wholmgren · 2021-05-19T03:06:37Z

pvlib/pvsystem.py

+        if len(self.arrays) > 1:
+            raise AttributeError(
+                f'{class_name}.{pvsystem_attr} not supported for multi-array '
+                f'systems. Use {alternative} instead.')


Consistent with my comment in modelchain.py, maybe something like:

{class_name}.{pvsystem_attr} not supported for multi-array systems. Set {array_attr} for each Array in {class_name}.arrays instead.

wholmgren · 2021-05-19T03:16:28Z

pvlib/pvsystem.py

    def module_type(self):
        return tuple(array.module_type for array in self.arrays)

    @property
    @_unwrap_single_value
+    @_check_deprecated_passthrough
    def temperature_model_parameters(self):
        return tuple(array.temperature_model_parameters
                     for array in self.arrays)

    @temperature_model_parameters.setter


The setters were added in #1076, so the deprecation requirement is a little more subtle. I think we either need to implement deprecation setters for each attribute or implement a __setattr__ like this from modelchain:

pvlib-python/pvlib/modelchain.py

Lines 503 to 510 in 7eae1fc

def __setattr__(self, key, value):

if key in ModelChain._deprecated_attrs:

msg = f'ModelChain.{key} is deprecated from v0.9. Use' \

f' ModelChain.results.{key} instead'

warnings.warn(msg, pvlibDeprecationWarning)

setattr(self.results, key, value)

else:

super().__setattr__(key, value)

For that matter we could implement a __getattr__ instead of properties, like this one:

pvlib-python/pvlib/modelchain.py

Lines 491 to 501 in 7eae1fc

def __getattr__(self, key):

if key in ModelChain._deprecated_attrs:

msg = f'ModelChain.{key} is deprecated and will' \

f' be removed in v0.10. Use' \

f' ModelChain.results.{key} instead'

warnings.warn(msg, pvlibDeprecationWarning)

return getattr(self.results, key)

# __getattr__ is only called if __getattribute__ fails.

# In that case we should check if key is a deprecated attribute,

# and fail with an AttributeError if it is not.

raise AttributeError

I don't know that one approach is better than another.

I'm not following the issue here, can you expand on what's different about the setters?

After trying to come up with an example, I'm realizing the problem is a little different than I first described.

There are currently setters for only 4 properties:

temperature_model_parameters

surface_tilt

surface_azimuth

racking_model

The remaining properties do not contain setters. At first I thought this meant that for all of the other properties, a user could set something on a PVSystem object that's different than what they'd get back from a getter that inspects the Arrays. But I'm now reminded that the user simply will not be able to set them.

In [5]: system = PVSystem([Array()]) In [6]: system.albedo = 0.3 --------------------------------------------------------------------------- AttributeError Traceback (most recent call last) <ipython-input-6-d89b04b86f95> in <module> ----> 1 system.albedo = 0.3 AttributeError: can't set attribute

So the issue is that the user can set some but not all of the deprecated attributes. I'd prefer if they could set none or all.

I brought up the __getattr__ and __setattr__ methods only as an alternative to the properties and setters.

So the issue is that the user can set some but not all of the deprecated attributes. I'd prefer if they could set none or all.

I see, and agree it should be consistent. I think omitting the setters was a (probably unintentional?) breaking change, so I think being able to set all (with a deprecation warning) makes sense. I've added setters where they were missing.

wholmgren · 2021-05-19T03:20:54Z

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

+  * ``PVSystem.surface_azimuth``
+  * ``PVSystem.racking_model``
+  * ``PVSystem.modules_per_string``
+  * ``PVSystem.strings_per_inverter``


I like the alphabetization of the list preceding this one and would prefer to see that here too.

wholmgren · 2021-05-19T03:33:36Z

Also a handful of problems in pvsystem.rst. I think you'll want to bring your branch up to date before tackling those.

wholmgren

@cwhanse ok to merge?

cwhanse · 2021-05-19T21:35:29Z

@cwhanse ok to merge?

There one warning in modelchain.py from codecov about a ValueError line not being hit, but it looks to me that the ValueError should have been raised by this test.

Other than that, LGTM.

kandersolar · 2021-05-20T21:25:07Z

There one warning in modelchain.py from codecov about a ValueError line not being hit, but it looks to me that the ValueError should have been raised by this test.

I'm not sure what I did wrong when I checked coverage earlier in the thread, but I now agree with codecov that the ValueError was not exercised. I checked coverage in pvlib==0.8.1 and it's not covered there either. The test you linked is for a similar but distinct error. 62a549e makes that test more specific and adds a test for the uncovered error as well.

cwhanse · 2021-05-20T21:41:46Z

Merge when green!

kandersolar added 3 commits March 13, 2021 12:36

add and apply decorator

1ee16e1

fix everything that broke

4d27feb

whatsnew

f5494aa

kandersolar marked this pull request as ready for review March 13, 2021 20:25

kandersolar mentioned this pull request Mar 13, 2021

add Mount classes #1176

Merged

9 tasks

wholmgren mentioned this pull request Mar 13, 2021

Add noct_sam cell temperature model to PVSystem, ModelChain #1195

Merged

8 tasks

remove unnecessary fixture, on the correct branch this time

9852c16

wholmgren added the api label Mar 29, 2021

wholmgren added this to the 0.9.0 milestone Mar 29, 2021

cwhanse mentioned this pull request May 5, 2021

pvlib v0.9 release planning #1214

Closed

24 tasks

cwhanse approved these changes May 6, 2021

View reviewed changes

kandersolar and others added 3 commits May 11, 2021 20:59

Update pvlib/modelchain.py

49635f4

Co-authored-by: Cliff Hansen <[email protected]>

reflow docstring

7588a81

more docstring updates

1824bd6

wholmgren reviewed May 19, 2021

View reviewed changes

kandersolar added 8 commits May 19, 2021 13:34

Merge remote-tracking branch 'upstream/master' into passthrough

8fba8a5

alphabetize

01099ed

rephrase error message

a29acae

pvsystem.rst edits

c775bc0

create missing PVSystem setters

7b96395

rephrase error message

71b07da

another pvsystem.rst edit

53ea45d

fix coverage drop

4619a1e

wholmgren approved these changes May 19, 2021

View reviewed changes

add missing test

62a549e

wholmgren merged commit 276a30d into pvlib:master May 20, 2021

kandersolar deleted the passthrough branch May 21, 2021 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate/raise error for PVSystem "pass-through" properties #1196

Deprecate/raise error for PVSystem "pass-through" properties #1196

kandersolar commented Mar 13, 2021 •

edited by wholmgren

Loading

kandersolar commented Mar 28, 2021

wholmgren commented Mar 29, 2021

kandersolar commented Mar 30, 2021

cwhanse May 6, 2021

kandersolar commented May 18, 2021

wholmgren May 19, 2021

wholmgren May 19, 2021

wholmgren May 19, 2021

kandersolar May 19, 2021

wholmgren May 19, 2021

kandersolar May 19, 2021

wholmgren May 19, 2021

wholmgren commented May 19, 2021

wholmgren left a comment

cwhanse commented May 19, 2021

kandersolar commented May 20, 2021

cwhanse commented May 20, 2021

	def __setattr__(self, key, value):
	if key in ModelChain._deprecated_attrs:
	msg = f'ModelChain.{key} is deprecated from v0.9. Use' \
	f' ModelChain.results.{key} instead'
	warnings.warn(msg, pvlibDeprecationWarning)
	setattr(self.results, key, value)
	else:
	super().__setattr__(key, value)

	def __getattr__(self, key):
	if key in ModelChain._deprecated_attrs:
	msg = f'ModelChain.{key} is deprecated and will' \
	f' be removed in v0.10. Use' \
	f' ModelChain.results.{key} instead'
	warnings.warn(msg, pvlibDeprecationWarning)
	return getattr(self.results, key)
	# __getattr__ is only called if __getattribute__ fails.
	# In that case we should check if key is a deprecated attribute,
	# and fail with an AttributeError if it is not.
	raise AttributeError

Deprecate/raise error for PVSystem "pass-through" properties #1196

Deprecate/raise error for PVSystem "pass-through" properties #1196

Conversation

kandersolar commented Mar 13, 2021 • edited by wholmgren Loading

kandersolar commented Mar 28, 2021

wholmgren commented Mar 29, 2021

kandersolar commented Mar 30, 2021

Choose a reason for hiding this comment

kandersolar commented May 18, 2021

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

wholmgren commented May 19, 2021

wholmgren left a comment

Choose a reason for hiding this comment

cwhanse commented May 19, 2021

kandersolar commented May 20, 2021

cwhanse commented May 20, 2021

kandersolar commented Mar 13, 2021 •

edited by wholmgren

Loading