Fix crash in ModelChain.infer_spectral_model due to incorrect method call (2) #629

jgaffiot · 2018-12-07T08:55:59Z

Closes modelchain: crash in ModelChain.infer_spectral_model due to incorrect method call #619
I am familiar with the contributing guidelines.
Fully tested. Added the test test_model_chain.test_infer_spectral_model to ensure correct behavior for all reasonable inputs.
[na] Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
[na] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Pull request is nearly complete and ready for detailed review.
Replace pull request Fix crash in ModelChain.infer_spectral_model due to incorrect method call #620 to have a clean history (I deleted my fork and created a new clean one).

stickler-ci · 2018-12-07T08:56:09Z

pvlib/test/test_modelchain.py

    return system


+@pytest.fixture


F811 redefinition of unused 'sam_data' from line 22

Same as lines 26, 40... I think this is a linter error.

stickler-ci · 2018-12-07T08:56:09Z

pvlib/test/test_modelchain.py

+    module = 'Canadian_Solar_CS5P_220M'
+    module_parameters = sam_data['cecmod'][module].copy()
+    inverters = sam_data['cecinverter']
+    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()


E501 line too long (80 > 79 characters)

Same as lines 32, 49... (which I copied). Should I change ?

By the way you may be interested by black to definitively get rid of formatting problems.

Fine with me to leave this because I'd like to refactor all of these fixtures.

Thanks for the tip.

jgaffiot · 2018-12-07T09:23:56Z

Travis error seems strange to me (only for Python3.6) :

ImportError while loading conftest '/home/travis/build/pvlib/pvlib-python/pvlib/test/conftest.py'.
pvlib/test/conftest.py:8: in <module>
    import pvlib
pvlib/__init__.py:12: in <module>
    from pvlib import spa
pvlib/spa.py:259: in <module>
    TABLE_1_DICT['L1'].resize((64, 3))
E   ValueError: cannot resize an array that references or is referenced
E   by another array in this way.  Use the resize function

Lack of "require_scipy" on a test ? Tests pass on my computer under Python3.6.7.

wholmgren

Looks good, but please add your name and/or username to the Contributors list at the bottom of the whatsnew file.

The test failure is unrelated and can be ignored.

wholmgren · 2018-12-07T14:15:39Z

pvlib/test/test_modelchain.py

    return system


+@pytest.fixture


wholmgren · 2018-12-07T14:17:28Z

pvlib/test/test_modelchain.py

+    module = 'Canadian_Solar_CS5P_220M'
+    module_parameters = sam_data['cecmod'][module].copy()
+    inverters = sam_data['cecinverter']
+    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()


Fine with me to leave this because I'd like to refactor all of these fixtures.

Thanks for the tip.

stickler-ci · 2018-12-07T14:32:53Z

pvlib/test/test_modelchain.py

    return system


+@pytest.fixture


F811 redefinition of unused 'sam_data' from line 22

stickler-ci · 2018-12-07T14:32:53Z

pvlib/test/test_modelchain.py

+    module = 'Canadian_Solar_CS5P_220M'
+    module_parameters = sam_data['cecmod'][module].copy()
+    inverters = sam_data['cecinverter']
+    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()


E501 line too long (80 > 79 characters)

cwhanse · 2018-12-07T16:03:02Z

pvlib/modelchain.py

                             'system.module_parameters. Check that the '
                             'parameters contain valid '
-                             'first_solar_spectral_coefficients or a valid '
+                             'first_solar_spectral_coefficients and a valid '


I still think this should be or. If Technology or Material is not in params, then attempt to identify the cell type. Or find 'first_solar_spectral_coefficients`.

Thanks @cwhanse for catching this. I agree that or describes the logic. @jgaffiot can you revert this change?

Unrelated to this change, but the error message also neglects the possibility of using sapm_spectral_loss.

I changed the comment because the code says 'and' line 624:

elif ((('Technology' in params or 'Material' in params) and (pvsystem._infer_cell_type() is not None)) or (self.system._infer_cell_type() is not None)) or 'first_solar_spectral_coefficients' in params):

I change the code too ?

No change to the code, only the comment.
(('Technology' in params or 'Material' in params) and (self.system._infer_cell_type() is not None))
is all one clause, that is OR-ed with the last clause. The and is inside. _infer_cell_type tries to set Technology and Material if those kwargs aren't given.

That compound logic statement could be presented more clearly using multiple lines.

stickler-ci · 2018-12-10T10:17:54Z

pvlib/test/test_modelchain.py

    return system


+@pytest.fixture


F811 redefinition of unused 'sam_data' from line 22

stickler-ci · 2018-12-10T10:17:54Z

pvlib/test/test_modelchain.py

+    module = 'Canadian_Solar_CS5P_220M'
+    module_parameters = sam_data['cecmod'][module].copy()
+    inverters = sam_data['cecinverter']
+    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()


E501 line too long (80 > 79 characters)

wholmgren · 2018-12-10T15:22:06Z

thanks @jgaffiot !

jgaffiot added 3 commits December 7, 2018 09:03

Fix incorrect call to pvsystem instead of self.system

de9f203

Update whatsnew

2231a04

Add a test for infer_spectral_model

4d99159

stickler-ci bot reviewed Dec 7, 2018

View reviewed changes

jgaffiot mentioned this pull request Dec 7, 2018

Fix crash in ModelChain.infer_spectral_model due to incorrect method call #620

Closed

5 tasks

wholmgren reviewed Dec 7, 2018

View reviewed changes

Add name to whatsnew

858e71f

stickler-ci bot reviewed Dec 7, 2018

View reviewed changes

wholmgren approved these changes Dec 7, 2018

View reviewed changes

cwhanse reviewed Dec 7, 2018

View reviewed changes

cwhanse approved these changes Dec 7, 2018

View reviewed changes

Revert change in comment

b65952e

stickler-ci bot reviewed Dec 10, 2018

View reviewed changes

wholmgren merged commit d979187 into pvlib:master Dec 10, 2018

wholmgren added this to the 0.6.1 milestone Jan 21, 2019

wholmgren added the bug label Jan 21, 2019

Fix crash in ModelChain.infer_spectral_model due to incorrect method call (2) #629

Fix crash in ModelChain.infer_spectral_model due to incorrect method call (2) #629

Uh oh!

Conversation

jgaffiot commented Dec 7, 2018 • edited by wholmgren Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickler-ci bot Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickler-ci bot Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

jgaffiot Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgaffiot commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickler-ci bot Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

stickler-ci bot Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwhanse Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickler-ci bot Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

stickler-ci bot Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

wholmgren commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jgaffiot commented Dec 7, 2018 •

edited by wholmgren

Loading

jgaffiot Dec 7, 2018 •

edited

Loading

jgaffiot commented Dec 7, 2018 •

edited

Loading

cwhanse Dec 7, 2018 •

edited

Loading