refactor bifacial merge, improve merge tests (#747)

* Simplify merge method.

Simplify merge method by using list and dictionary comprehensions instead of
nested for loops. This also avoids the need to check if some of the elements
of the reports arguments are None, and the need to check if reports contains
more than one report. This should also be a small performance improvement.
This change also adds a bit more detail to the docstring for the merge method.
The creation of a new empty report is also modified to no longer use an
intermediary list of keys.

* Update what's new doc.

* Update whatsnew file syntax.

* Rename whatsnew file to change format to rst.

* Fix linting errors.

* Remove pandas and numpy version reqs

* Update v0.7.1.rst

* Revert "Update v0.7.1.rst"

This reverts commit d131657.

* Revert "Remove pandas and numpy version reqs"

This reverts commit 84d7189.

* Reword elements in merge() comprehension.

* Fix handling of None values in merge.

If one of the values in the reports argument that merge() takes is a None,
this is now properly handled by being dropped.

* Remove unnecessary checks in merge().

Returns merge method to its original state with two nested for loops, but without
the checks for the number of dictionaries in the reports argument, or the check
that each element in reports is a dictionary. Reports are only created in the build
method so a test was made to ensure that build always returns a dictionary instead.

* Add tests for merge() and build().

* Fix lint errors.

* Correct import mistake.

Previous commits I made did not import the PVFactorsReportBuilder
class correctly.

* Reinsert type check in merge loop.

* Revert "Reinsert type check in merge loop."

This reverts commit eeb8053.

* Fix test_build_1 test.

* Update what's new file.

Add bullet point about added tests for bifacial.py methods.

* Fix merge conflicts in what's new file.

* Revert "Fix merge conflicts in what's new file."

This reverts commit 74c8eba.

* Revert "Update what's new file."

This reverts commit 4447e6c.

* Revert "Rename whatsnew file to change format to rst."

This reverts commit 93d0559.

* Revert "Update whatsnew file syntax."

This reverts commit c969994.

* Revert "Update what's new doc."

This reverts commit 55d8f08.

* Merge what's new updates from upstream.

* Update what's new file.

Author: alexandermorgan

docs/sphinx/source/whatsnew/v0.7.0.rst

@@ -7,7 +7,7 @@ This is a major release that drops support for Python 2 and Python 3.4. We
 recommend all users of v0.6.3 upgrade to this release after checking API
 compatibility notes.
 
-**Python 2.7 support ended on June 1, 2019**. (:issue:`501`)
+**Python 2.7 support ended on June 1, 2019.** (:issue:`501`)
 **Minimum numpy version is now 1.10.4. Minimum pandas version is now 0.18.1.**
 
 Bug fixes
@@ -18,10 +18,12 @@ Bug fixes
 Testing
 ~~~~~~~
 * Added 30 minutes to timestamps in `test_psm3.csv` to match change in NSRDB (:issue:`733`)
+* Added tests for methods in bifacial.py.
 
 
 Contributors
 ~~~~~~~~~~~~
 * Mark Campanellli (:ghuser:`markcampanelli`)
 * Will Holmgren (:ghuser:`wholmgren`)
 * Oscar Dowson (:ghuser:`odow`)
+* Alexander Morgan (:ghuser:`alexandermorgan`)

pvlib/bifacial.py

@@ -159,8 +159,7 @@ def build(report, pvarray):
         back surface of center pvrow (index=1)"""
         # Initialize the report as a dictionary
         if report is None:
-            list_keys = ['total_inc_back', 'total_inc_front']
-            report = {key: [] for key in list_keys}
+            report = {'total_inc_back': [], 'total_inc_front': []}
         # Add elements to the report
         if pvarray is not None:
             pvrow = pvarray.pvrows[1]  # use center pvrow
@@ -177,13 +176,12 @@ def build(report, pvarray):
 
     @staticmethod
     def merge(reports):
-        """Works for dictionary reports"""
+        """Works for dictionary reports. Merges the reports list of
+        dictionaries in a single dictionary. The list of the first
+        dictionary are extended by those of all subsequent lists."""
         report = reports[0]
-        # Merge only if more than 1 report
-        if len(reports) > 1:
-            keys_report = list(reports[0].keys())
-            for other_report in reports[1:]:
-                if other_report is not None:
-                    for key in keys_report:
-                        report[key] += other_report[key]
+        keys_report = list(report.keys())
+        for other_report in reports[1:]:  # loop won't run if len(reports) < 2
+            for key in keys_report:
+                report[key] += other_report[key]
         return report

pvlib/test/test_bifacial.py

@@ -1,6 +1,7 @@
 import pandas as pd
+import numpy as np
 from datetime import datetime
-from pvlib.bifacial import pvfactors_timeseries
+from pvlib.bifacial import pvfactors_timeseries, PVFactorsReportBuilder
 from conftest import requires_pvfactors
 import pytest
 
@@ -107,3 +108,43 @@ def test_pvfactors_timeseries_pandas_inputs(run_parallel_calculations):
 
     pd.testing.assert_series_equal(ipoa_front, expected_ipoa_front)
     pd.testing.assert_series_equal(ipoa_back, expected_ipoa_back)
+
+
+def test_build_1():
+    """Test that build correctly instantiates a dictionary, when passed a Nones
+    for the report and pvarray arguments.
+    """
+    report = None
+    pvarray = None
+    expected = {'total_inc_back': [np.nan], 'total_inc_front': [np.nan]}
+    assert expected == PVFactorsReportBuilder.build(report, pvarray)
+
+
+def test_merge_1():
+    """Test that merge correctly returns the first element of the reports
+    argument when there is only dictionary in reports.
+    """
+    test_dict = {'total_inc_back': [1, 2, 3], 'total_inc_front': [4, 5, 6]}
+    reports = [test_dict]
+    assert test_dict == PVFactorsReportBuilder.merge(reports)
+
+
+def test_merge_2():
+    """Test that merge correctly combines two dictionary reports.
+    """
+    test_dict_1 = {'total_inc_back': [1, 2], 'total_inc_front': [4, 5]}
+    test_dict_2 = {'total_inc_back': [3], 'total_inc_front': [6]}
+    expected = {'total_inc_back': [1, 2, 3], 'total_inc_front': [4, 5, 6]}
+    reports = [test_dict_1, test_dict_2]
+    assert expected == PVFactorsReportBuilder.merge(reports)
+
+
+def test_merge_3():
+    """Test that merge correctly combines three dictionary reports.
+    """
+    test_dict_1 = {'total_inc_back': [1], 'total_inc_front': [4]}
+    test_dict_2 = {'total_inc_back': [2], 'total_inc_front': [5]}
+    test_dict_3 = {'total_inc_back': [3], 'total_inc_front': [6]}
+    expected = {'total_inc_back': [1, 2, 3], 'total_inc_front': [4, 5, 6]}
+    reports = [test_dict_1, test_dict_2, test_dict_3]
+    assert expected == PVFactorsReportBuilder.merge(reports)