Expose first_solar_spectral_correction

[cwhanse]

pvlib python pull request guidelines

Thank you for your contribution to pvlib python!

You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:

• Closes Expose first_solar_spectral_correction in ModelChain and PVSystem #359
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
• Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• 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.

Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

#359 complete

[wholmgren]

This is great!

'fs_spectral_coefficients' or 'first_solar_spectral_coefficients'? The latter is pretty long, but users that are less familiar with the library might be tripped up by the abbreviation. I lean towards spelling it out for consistency with the function/method calls.

The PVSystem._infer_cell_type is a nice addition. I think you're right to make it a private method for now. But I would reconsider if anyone reading this says that they would make use of it now.

We could call self._cell_type = self._infer_cell_type() as part of the PVSystem init. Or self.cell_type. Probably not a great idea, just thinking aloud.

Should be relatively straight forward to add a few simple tests for PVSystem.first_solar_spectral_correction. Updating the ModelChain tests is going to be a little painful. Many of the values will change now that this correction is applied by default. I can look at that more carefully later.

[wholmgren]

module vs. cell is unfortunate here. No objection to what you've done, but we might need a separate discussion to try to clean up the terminology within the library.

[cwhanse]

Here I'm inclined to break the API and change the keyword argument from module_type to cell_type. module_type is only used by the first_solar function.

[wholmgren]

Recommend removing this if/else statement. If this method doesn't have the information necessary to successfully compute the spectral correction, then this method should say so rather than defaulting to 1.

Consistent with Python's style, the atmosphere.first_solar_spectral_correction function throws a TypeError with some hint as to what the user needs to supply. We could (ideally/optionally) catch that error here and reraise it with a more specific message for how to modify module_parameters to make this work.

I am guessing that raising an exception here will cause issues in a ModelChain calculation for the few module/cell types for which _infer_cell_type mapping returns None. One approach to avoid that may be to make the ModelChain.infer_spectral_model preemptively call pvsystem._infer_cell_type.

[cwhanse]

Good idea. I'll move the test to ModelChain.infer_spectral_model and put a warning in place of that check in the actual functions.

[wholmgren]

typo (is can) that is carried over from the original function's doc string.

[adriesse]

I looked it over and didn't find anything worth bothering you about. :)

[cwhanse]

I'm tripping over function name conventions again. Related to this PR, but maybe it merits its own Issue and PRs. For spectral modifier models, we have first_solar_spectral_loss and sapm_spectral_loss, where the model name is the prefix to the model purpose. We did likewise for physicaliam and ashraeiam . For clear-sky models we conveniently bundled the models haurwitz etc. into a module (is that the right term?) so that the path provides the prefix, e.g., clearsky.haurwitz

I have a strong preference to have the model purpose as the prefix, followed by the model name, e.g., spectral_loss_first_solar and iam_physical. Or we could create a separate module for each model purpose, e.g., spectral_loss.first_solar.

Thoughts?

[wholmgren]

Good point. I also prefer the naming schemes you describe.

Perhaps the easiest way forward for this PR is to continue with PVSystem.first_solar_spectral_loss for consistency with the related function and with the other PVSystem methods. It's easy to change the name in a future PR within the same release cycle.

This is also related to the discussion in #436. And #427 adds machinery for deprecating function names without immediately breaking user code.

[cwhanse]

I remember that discussion. I'll complete this PR with function names as is, let's tackle the naming issue in a separate PR.

@wholmgren I'm going to need some guidance on implementing the tests. Looks like we need a new test_first_solar_spectral_loss in test_pvsystem. I can generate the correct value for different cell types in MATLAB. What's a reasonable value for pw?

I do not understand the code in the current test_modelchain.test_spectral_models It looks like we are passing arguments from the aoi functions.

[wholmgren]

If you want to compare to matlab, I recommend ensuring that the first_solar_spectral_loss function tests in test_atmosphere.py produce the same results.

For PVSystem.first_solar_spectral_loss method tests I am more concerned with API correctness. One good approach may be to set up your test PVSystem object with the module_parameters you want, then ensure that the PVSystem.first_solar_spectral_correction produces the same result as an explicit call to atmosphere.first_solar_spectral_correction. Then repeat the process with different configurations. Repeating the process is best done using the @pytest.mark.parametrize decorator, though I recommend starting simple. It's ok with me if you want to copy/paste 3 or 4 similar tests to get started.

[cwhanse]

@wholmgren when you can, a little help to understand why these tests are failing? I have the decorator wrong but I don't understand why.

[wholmgren]

Traveling this weekend. Will check Tuesday.

…

On Sat, May 26, 2018 at 2:26 PM Cliff Hansen ***@***.***> wrote: @wholmgren <https://github.com/wholmgren> when you can, a little help to understand why these tests are failing? I have the decorator wrong but I don't understand why. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELiR5FQxTEmiW8poM3lPrCHfDCFPZl9ks5t2ch3gaJpZM4UKygZ> .

[cwhanse]

Progress. I grok the test now. The code I submitted is following the pattern for test_aoi_models. It defines a ModelChain object, uses the run_model method and inspects the output. I had an error that I was examining the ac output instead of the spectral_modifier output. Now I'm up against a design decision for the test.

When we test individual functions like the spectral models we have to know the expected output. So I made up values for precipitable_water and airmass_absolute and computed the values each spectral model should produce. However, we are testing these functions as part of a model chain, where the output of the function being tested depends on the output of upstream functions. In this case, the output of sapm_spectral_loss depends on airmass which is calculated using the times input, ignoring the made-up values for precipitable_water and airmass_absolute I supplied when the model chain is created in test_spectral_models.

I see three ways to resolve the problem, neither of which are appealing:

1. I recalculate the expected values by running each model in the chain. I don't like this approach, because a future change to one of the upstream functions could cause this test of the spectral models to fail.
2. Change the test pattern to not use a ModelChain object. This is a problem because it wouldn't use the ModelChain methods which assign the spectral model method to the ModelChain object.
3. Add code to ModelChain.__init__ to handle airmass as a kwarg, and impose user-supplied values in place of the calculation using times and location. This is my least liked option because I don't see a use of the additional code beyond this made-up test.

What is preferred here? I'll proceed with the first option, but perhaps there's a fourth option I don't see.

[adriesse]

Perhaps it is not necessary to check the precise values that are returned using the ModelChain. If the spectral calculation function is already verified to be correct through its own tests, then you really just need to test/exercise the additional code related to the ModelChain integration. It might be enough to check that you get a Series of floats or something like that, I'm not sure.

I suppose the inference code would need to be tested somehow too. That might be easier if the result of any inferences were explicitly stored in the ModelChain, so a test (or a user) could check what happened.

[wholmgren]

Let me first try to state my current thinking about how to structure unit tests in pvlib in the context of this pull request.

1. The tests in test_atmosphere.py should ensure that the atmosphere.first_solar_spectral_correction function returns the desired output for a variety of function inputs. The tests should be independent of other pvlib functions (see Unit tests depend on functions not being tested #394). The tests should ensure that all reasonable combinations of input types (floats, nans, arrays, series, scalars, etc) work as expected. I think the existing tests for this function are reasonably good and no new work is needed in this pull request.

2. The tests in test_pvsystem.py should ensure that the PVSystem.first_solar_spectral_correction method calls the atmosphere.first_solar_spectral_correction function with the desired inputs determined from all relevant configurations of a PVSystem object. A test should also ensure that the method does return some reasonable data, though the precise values of the data should be covered by tests in item 1.

3. The tests in test_modelchain.py should ensure that ModelChain.__init__ correctly configures the ModelChain object to eventually run PVSystem.first_solar_spectral_correction. A test should ensure that method is eventually run in the course of Model_chain.run_model. A test should ensure that the model selection does have a reasonable effect on the subsequent calculations, though the precise values of the data should be covered by tests in item 1.

That thinking has evolved from the many mistakes that I have made in writing pvlib tests, too many of which are still with us.

The function-level tests in item 1 are relatively straight forward so long as we avoid the dependency issue @cwhanse pointed out in #394. I used to be bad and lazy about that, so there are some older tests that have dependencies that they shouldn't have.

The PVSystem and ModelChain tests are less straight forward to implement. I often thought that I could reasonably check items 2 and 3 by testing that a subset of user-level numeric inputs produced the desired numeric outputs. Of course, these higher-level objects effectively inject a bunch of dependencies into the tests, and we're back at issue #394. The plus side is that they are simple to write the first time.

Moving forward to comments from @cwhanse and @adriesse

I recalculate the expected values by running each model in the chain. I don't like this approach, because a future change to one of the upstream functions could cause this test of the spectral models to fail.

Agreed.

Change the test pattern to not use a ModelChain object. This is a problem because it wouldn't use the ModelChain methods which assign the spectral model method to the ModelChain object.

We will want tests that only use a PVSystem object (item 2) and tests that do use a ModelChain object (item 3).

Add code to ModelChain.init to handle airmass as a kwarg, and impose user-supplied values in place of the calculation using times and location. This is my least liked option because I don't see a use of the additional code beyond this made-up test.

Agreed -- I think we can come up with a better way.

Perhaps it is not necessary to check the precise values that are returned using the ModelChain. If the spectral calculation function is already verified to be correct through its own tests, then you really just need to test/exercise the additional code related to the ModelChain integration.

Agreed, though I do want to emphasize the importance (in my mind) of separating tests at the level of PVSystem (item 2) and ModelChain (item 3). I don't know how to write this kind of test, though, and welcome suggestions. I know it's possible to assert that a function is called (maybe even with specific arguments), but I don't have practice doing it. I think pytest has some support for this kind of testing. The mock library is also useful for related testing problems.

I suppose the inference code would need to be tested somehow too. That might be easier if the result of any inferences were explicitly stored in the ModelChain, so a test (or a user) could check what happened.

The inferences are stored in the sense that a specific model's corresponding method e.g. ModelChain.first_solar_spectral_loss is assigned to the corresponding generic attribute e.g. ModelChain._spectral_model. A test could ensure that this happens given some input PVSystem configurations. Also, the string representation of a ModelChain allows a user to easily check what happened with the inference using print(my_model_object).

How to move forward? I will start by learning more about pytest and mock in an effort to design test templates that let us address items 2 and 3 while avoiding unintended dependencies. I'll look into that this week, though it may take longer to come up with a good solution. I am also ok moving forward with something like @cwhanse's option 1, but it's complicated by the fact that some of the existing model chain tests break due to the corrections applied by this new feature.

Finally, we should add some text and examples to the contributing section of the documentation if we can come to some agreement about testing principles and practice.

[wholmgren]

This might be easier than I thought... See this commit for an example of using pytest-mock with the PVSystem incidence angle modifiers: wholmgren@783ddf3

I don't fully understand the pros/cons of using pytest-mock vs. pure mock, but this seems doable either way.

[cwhanse]

@wholmgren many thanks for your thoughts. I haven't yet looked at the pytest-mock example. In the interim, in the interest of getting something working, I changed the pvsystem module test to verify that the PVSystem method returns one correct (precalculated) value, and the modelchain module test to only verify that the ModelChain method returns a Series, float or int types. I think this is the spirit of the test distinctions you outline, and if this works, we can certainly add more conditions to each test.

[wholmgren]

I think this is nearly ready. I made some minor comments below. We'll also need an entry in the whats new file.

[wholmgren]

python convention is no blank lines between decorators and the function.

[cwhanse]

Fixed

[wholmgren]

I try to avoid putting print functions in the final tests. In case you are not already aware of it, the pytest --pdb option can be useful for avoiding print in development e.g. pytest pvlib/test/test_modelchain.py::test_spectral_models --pdb

[cwhanse]

Removed, I had that print to debug the test.

[wholmgren]

please keep constant_spectral_loss in the list of parameters. This tests that a user-supplied function can be passed in as an argument. To be clear, the list should be: ['sapm', 'first_solar', 'no_loss', constant_spectral_loss]

[adriesse]

constant_spectral_loss --> constant_loss ?

[cwhanse]

I put the keyword back. I tend to agree with @adriesse about 'loss' but the terminology is common. Maybe the time to alter usage is when (if) we revise function naming patterns, because changing 'loss' will alter the API.

[wholmgren]

Doc string should have some kind of reference to self.module_parameters. Most of the other PVSystem method doc strings have some examples. This method is a little more complicated though, so might be worth being more explicit about what is looked for. For example...

Use the :py:func:first_solar_spectral_correction function to calculate the spectral loss modifier. The spectral loss modifier is determined by searching for one of the following keys in self.module_parameters (in order): 'first_solar_spectral_coefficients', 'Technology', 'Material'.

[cwhanse]

    Use the :py:func:`first_solar_spectral_correction` function to
    calculate the spectral loss modifier. The model coefficients are
    specific to the module's cell type, and are determined by searching
    for one of the following keys in self.module_parameters (in order):
        'first_solar_spectral_coefficients (user-supplied coefficients)
        'Technology' - a string describing the cell type, can be read from
        the CEC module parameter database
        'Material' - a string describing the cell type, can be read from
        the Sandia module database.

[adriesse]

While browsing the code, it struck me that the word "loss" is used quite a lot. In contrast to many other loss types, this one can be a gain also and pvlib should perhaps reflect this by using a more neutral word (or "gain/loss"). Only where it is convenient to do so of course.

[wholmgren]

Minor typo, then we are ready to merge.

[wholmgren]

missing ' -

[wholmgren]

@adriesse I am open to discussing and changing the "loss" terminology as part of a separate issue.