1759 resolve attrib error model chain

[matsuobasho]

• Closes AttributeError when using ModelChain with multiple Arrays #1759
• I am familiar with the contributing guidelines
• 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`).
• 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.

[matsuobasho]

Any updates on this?

[cwhanse]

@pvlib/pvlib-maintainer please take a look. The PR moves pvlib towards requiring the temperature model to be explicit, either by providing parameters or setting racking_model and module_type. #1938 (closed) took a larger bite, which we decided to rewind. #1942 and #1946 identify improvements needed in some error messages.

[kandersolar]

Thanks @matsuobasho!

Can we add a test? Something like this (please check that it makes sense) around line 1308 in test_modelchain.py:

def test_temperature_model_not_specified():
    # GH 1759 -- ensure correct error is raised when temperature model params
    # are specified on the PVSystem instead of the Arrays
    location = Location(latitude=32.2, longitude=-110.9)
    arrays = [pvsystem.Array(pvsystem.FixedMount(),
                             module_parameters={'pdc0': 1, 'gamma_pdc': 0})]
    system = pvsystem.PVSystem(arrays,
                               temperature_model_parameters={'u0': 1, 'u1': 1},
                               inverter_parameters={'pdc0': 1})
    with pytest.raises(ValueError,
                       match='could not infer temperature model '
                             'from system.temperature_model_parameters'):
        _ = ModelChain(system, location,
                       aoi_model='no_loss', spectral_model='no_loss')

[kandersolar]

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

[matsuobasho]

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.

[kandersolar]

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 :)

[matsuobasho]

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.

[kandersolar]

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.

[cwhanse]

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.

[matsuobasho]

@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.

[kandersolar]

Thanks again @matsuobasho! This is a good bug to have fixed.

[matsuobasho]

Glad to help! Thanks for your inputs @cwhanse and @kandersolar