@wholmgren @wfvining one test is failing. I actually think the ModelChain should run with that that test setup, since effective_irradiance is provided for every array. The cell temperature model can use effective_irradiance since #1129. PTAL and let me know if you agree.

That does seem reasonable to me. If we remove that test, we should make sure that there is still a test that covers the case where one array does not have enough information to calculate cell temperature.

I agree.

That does seem reasonable to me. If we remove that test, we should make sure that there is still a test that covers the case where one array does not have enough information to calculate cell temperature.

I had to call _set_celltemp directly to get a case that raises the ValueError. Missing 'poa_global' is covered by the new test__set_celltemp_missing_poa. Missing 'poa_global' or effective_irradiance are caught upstream by the _check_multiple_input steps of run_from_poa and run_from_effective_irradiance.

I had to call _set_celltemp directly to get a case that raises the ValueError

This suggests to me that the ValueError is only accessible from private calls and cannot be hit through run_* method calls. Am I misunderstanding?

I had to call _set_celltemp directly to get a case that raises the ValueError

This suggests to me that the ValueError is only accessible from private calls and cannot be hit through run_* method calls. Am I misunderstanding?

I believe that to be the case, but could be wrong.

Why keep it if we can't hit it through the run method calls?

I had to call _set_celltemp directly to get a case that raises the ValueError

This suggests to me that the ValueError is only accessible from private calls and cannot be hit through run_* method calls. Am I misunderstanding?

• With ModelChain.run_model, prepare_inputs calls _check_multiple_input to verify that weather has the same dimension as num_arrays, and calls _verify_df to verify that 'ghi', 'dhi' and 'dni' are present in weather.
• With ModelChain.run_model_from_poa, prepare_inputs_from_poa calls _check_multiple_input to verify that data has the same dimension as num_arrays, and _verify_df to verify that data contains 'poa_global', 'poa_direct' and 'poa_diffuse'.
• With ModelChain.run_model_from_effective_irradiance, there is no prepare_inputs method. _check_multiple_input is called to verify that data has the same dimension as num_arrays. There is no use of _verify_df to check for effective_irradiance in data. Seems like an oversight here.

All three public run_model methods end up at _run_from_effective_irrad, which calls _prepare_temperature and that's how execution gets to _set_celltemp.

Why keep it if we can't hit it through the run method calls?

Good question. I'm OK removing that check. @wfvining ?

There is no use of _verify_df to check for effective_irradiance in data. Seems like an oversight here.

I agree.

Maybe out of scope for this PR, but I think we could refactor _prepare_temperature to use _verify_df in try/except blocks that parallel the logic in run_model_from_effective_irradiance docstring. I still struggle to understand _prepare_temperature every time I come back to it. We also now have the benefit of a ModelChainResults setter that will ensure the output type is correct.

We also need to add or change a test to cover the specific case in #1163. All of the test_run_model_from_effective_irradiance_* functions include POA data, while #1163 only has effective irradiance. That might be why we're not hitting the error from the run method calls - not confident in that.

#1163 is test_run_model_from_effective_irradiance_no_poa_global but with an iterable input.

Why keep it if we can't hit it through the run method calls?

Good question. I'm OK removing that check. @wfvining ?

I'm okay removing it or just making it catch any generic exception as opposed to a specific ValueError. I think given the amount of state maintained inside ModelChain, once missing data is allowed inside of modelchain it can be very difficult to notice or debug problems, so these kinds of tests are pretty valuable.

Maybe out of scope for this PR, but I think we could refactor _prepare_temperature to use _verify_df in try/except blocks that parallel the logic in run_model_from_effective_irradiance docstring. I still struggle to understand _prepare_temperature every time I come back to it

I would support refactoring this. My guess is that the bulk of the technical debt in ModelChain is related to _prepare_temperature.

I think this should do it:

@pytest.mark.parametrize("input_type", [lambda x: x[0], tuple, list])
def test_run_model_from_effective_irradiance_no_poa_global(
        sapm_dc_snl_ac_system, location, weather, total_irrad, input_type):
    data = weather.copy()
    data['effective_irradiance'] = total_irrad['poa_global']
    mc = ModelChain(sapm_dc_snl_ac_system, location, aoi_model='no_loss',
                    spectral_model='no_loss')
    ac = mc.run_model_from_effective_irradiance(input_type((data,))).results.ac
    expected = pd.Series(np.array([149.280238, 96.678385]),
                         index=data.index)
    # if isinstance(ac, (tuple, list)):
    #     ac = ac[0]
    assert_series_equal(ac, expected)

edit: I commented out the if isinstance(ac, (tuple, list)) check because ac is always a Series.

I would support refactoring this.

As would I. New issue though.

I would also propose that we consider combining _check_multiple_inputs and _verify_df to hopefully make these checks more easily understood in the future. The only separate use of this pair of methods is in complete_irradiance which doesn't use _verify_df and instead explicitly looks for 2 of 'ghi', 'dhi' and 'dni'. Maybe that check could be built into _verify_df.

Ok to merge