Repository: pvlib/pvlib-python
Pull request #1162 (2021-02-05 18:29:54)

Make sure times are assigned with ModelChain.run_from_poa
<!-- Thank you for your contribution! The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items. Feel free to remove checklist items that are not relevant to your change. -->

 - [ ] Closes #xxxx
 - [x] I am familiar with the [contributing guidelines](https://pvlib-python.readthedocs.io/en/latest/contributing.html)
 - [x] Tests added
 - [ ] Updates entries to [`docs/sphinx/source/api.rst`](https://github.com/pvlib/pvlib-python/blob/master/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`](https://github.com/pvlib/pvlib-python/tree/master/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](https://numpydoc.readthedocs.io/en/latest/format.html) compliant docstrings, examples, and comments where necessary.
 - [x] Pull request is nearly complete and ready for detailed review.
 - [x] Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

<!-- Brief description of the problem and proposed solution (if not already fully described in the issue linked to above): -->

When running `ModelChain.run_from_poa`, `ModelChain.times` is not assigned so that the solar position results are incorrect and have a `NaT` index.

The issue was introduced in #1076 here https://github.com/pvlib/pvlib-python/blob/b0d29f550ea14d49499a97d60fa80c32a2a883d6/pvlib/modelchain.py#L1196-L1203, so I'm not sure if any what's new update is needed.

wholmgren: looks like the same issue exists in `ModelChain.run_model_from_effective_irradiance`. `times` is not separable from `weather`, so maybe it should be called within `_assign_weather`.
alorenzo175: Yes it is missing there too, but I guess not a problem since it doesn't compute and assign solar position. So move it to `_assign_weather`?
cwhanse: @wholmgren it appears that the only use of `ModelChain.times` is as input to a solar position calculation [here](https://github.com/pvlib/pvlib-python/blob/5a5d4aa7e5e5942b170b05f6acdad8830e7dc6a7/pvlib/modelchain.py#L1185) and [here](https://github.com/pvlib/pvlib-python/blob/5a5d4aa7e5e5942b170b05f6acdad8830e7dc6a7/pvlib/modelchain.py#L1236). I image those calls used `weather.index` before the multi-array capability. I'm not sure that `ModelChain.times` is consistent with the rest of `ModelChain` attributes, and if it persists is likely to get baked into user applications because it is convenient.

I'm OK patching this now so that pvlib behaves consistently. But down the road I think we'll want to remove `ModelChain.times` in favor of perhaps `ModelChain.results.times`?
wholmgren: I agree. A year ago or so I wanted to remove `ModelChain.times` entirely because it no longer served any purpose, but refactorings since then have brought it back to life. `ModelChain.weather` should also be moved to the results. 

Let's move it to `_assign_weather` as a short term fix.

Patch:
diff --git a/pvlib/modelchain.py b/pvlib/modelchain.py
index dbca739..6a8903b 100644
--- a/pvlib/modelchain.py
+++ b/pvlib/modelchain.py
@@ -1324,6 +1324,7 @@ class ModelChain:
                 _build_weather(weather) for weather in data
             )
         self._configure_results()
+        self._assign_times()
         return self
 
     def _assign_total_irrad(self, data):
@@ -1392,7 +1393,6 @@ class ModelChain:
         self._check_multiple_input(weather, strict=False)
         self._verify_df(weather, required=['ghi', 'dni', 'dhi'])
         self._assign_weather(weather)
-        self._assign_times()
 
         self._prep_inputs_solar_pos(weather)
         self._prep_inputs_airmass()


Test patch:
diff --git a/pvlib/tests/test_modelchain.py b/pvlib/tests/test_modelchain.py
index 3b48ddf..1e627b5 100644
--- a/pvlib/tests/test_modelchain.py
+++ b/pvlib/tests/test_modelchain.py
@@ -774,6 +774,7 @@ def test_prepare_inputs_from_poa(sapm_dc_snl_ac_system, location,
     assert_frame_equal(mc.weather, weather_expected)
     # total_irrad attribute
     assert_frame_equal(mc.results.total_irrad, total_irrad)
+    assert not pd.isnull(mc.results.solar_position.index[0])
 
 
 @pytest.mark.parametrize("input_type", [tuple, list])