refactor bifacial merge, improve merge tests

[alexandermorgan]

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.

pvlib python pull request guidelines

• Closes issue
• I am familiar with the contributing guidelines.
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
• 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.
• 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.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
This does not close a pre-existing issue, but does optimize and simplify the merge function in bifacial.py.

[cwhanse]

I'm not at all familiar with bifacial.py. Is it clear that reports[0].keys() contains all the key values that any element of reports might have?

[alexandermorgan]

That's what the earlier implementation was doing anyway, so it's as sure as it was before. Each report seems to be coming from the build method just above the merge method, and if I'm correct about that, then each report dictionary in reports would have the same two keys.

[wholmgren]

@anomam can you comment on this change? I want to make sure we're not missing something. If we are missing something, then we probably need a new test to catch it.

I like comprehensions, but I'm not convinced that this change is a win for readability.

[mikofski]

Can you provide a test please? Also is it possible to use itertools.chain()? I don't think so, but maybe.

>>> a = [
...     {'a': [1, 2], 'b': [2, 3], 'c': [3, 4]},
...     {'a': [44, 55], 'b': [55, 66], 'c': [66, 77]},
...     {'a': [777, 888], 'b': [888, 999], 'c': [999, 1000]}]
>>> {k: [x for y in a for x in y[k]] for k in a[0].keys()}
a = [{'a':[1,2],'b':[2,3],'c':[3,4]}, {'a':[44,55],'b':[55,66], 'c':[66,77]}, {'a':[777,888],'b':[888,999],'c':[999,1000]}]

works, but I get TypeError: 'NoneType' object is not subscriptable if None is in a

[anomam]

Since all reports are currently initialized with this (line 162)

report = {'total_inc_back': [], 'total_inc_front': []}

the NoneType error shouldn't be a problem. And it doesn't look to me like it's increasing the risk of having a bug compared to before. But I wouldn't be against a test for sure!
I agree that we lose in readability, but if the gain in performance is worth it, I think we should consider it. But it's up to the pvlib admins!
I don't think I will port this to the pvfactors package because of the readability concern, and because the reports in pvfactors are just there as examples; I use custom-made report classes in my work (and sometimes custom-made irradiance model classes) with pvfactors. Maybe that's something we could allow in pvlib as well?

[wholmgren]

What is the gain in performance in this case?

[alexandermorgan]

@cwhanse The Travis CI build seems to have failed for Python 3.5 for the same reason it failed in the master build 11 days ago (it can't find the pandas and numpy packages). I don't see how this could have been because of my small changes, and all the other tests passed, so unless you think otherwise, I'm going to assume that my pull request is not causing any issues.

[cwhanse]

my pull request is not causing any issues.

Agree, your PR is not at fault. The minimum build is failing for all PRs.

[cwhanse]

@alexandermorgan is the latest commit needed if you merge #748 into this PR?

[alexandermorgan]

@cwhanse no, I don't need my last commit. Thanks, I hadn't noticed PR #748. I'll merge it into this pull request and revert my last two commits. I take it there is some reason for wanting such an old version of pandas? Pandas 0.24 is compatible with python 3.5.

[cwhanse]

#748 is merged so you can merge master. I think the idea with py35-min is lowest workable versions.

[alexandermorgan]

Ok, this should be all set once the tests finish. I have a fair amount of experience with Pandas, and also optimizing Python code in general. I'm trying out some new optimization strategies right now, so if there is a heavily used block of code that you think could be sped up, I'd love to give it a try. This is a really cool project so I'd like to contribute more.

[cwhanse]

@alexandermorgan maybe clearsky.lookup_linke_turbidity? That function is often at the top of the profiler list. Not so much a pandas usage, but rather, loading a large data file.

[wholmgren]

this should be in 0.7.0, right?

[alexandermorgan]

My mistake, I thought 7.0 was already released. I'll correct this.

[alexandermorgan]

@wholmgren I think the what's new file is all set now. I don't quite understand what about it caused the LGTM to fail. I think this PR is good to go.

[wholmgren]

@anomam can you comment on this change? I want to make sure we're not missing something. If we are missing something, then we probably need a new test to catch it.

I like comprehensions, but I'm not convinced that this change is a win for readability.

[alexandermorgan]

@wholmgren I agree that tests are needed for these changes. There currently aren't any tests for the merge method. I'll add some tomorrow to cover the main cases (probably where there are 0, 1, 2, or 3 reports, and where one of the reports passed is a None). What would be the desired behavior if reports[0] is None and len(reports) == 1? Currently, this would just return a None. Also, what is the desired behavior if reports is an empty list? Currently, this would throw an error, but I'm not sure if that is a possible input in the program.
Concerning readability, I'll change "d" (which stood for dictionary) to "report", and also "k" to "key" to make the comprehensions clearer. It's a fair critique that comprehensions are generally seen as less readable than for loops. But here the gains are pretty solid. The complexity is much lower because only one list is created per key and all at once instead of each list constantly getting extended via the + operator. In best/avg./worst cases, this should be a win, and it should be a big improvement in the worst case. Also, the comprehensions handle None values and any number of reports >= 1, so a couple of logic checks are no longer needed.

[alexandermorgan]

I tried a lot of different ways to combine the dicts in merge(), but couldn't find one that was better than what you already had. This does make two minor changes to that method though. It removes the unnecessary len(reports) check. In py3, slicing a list out of range doesn't throw an error, just an empty list, so looping over reports[1:] just won't enter the loop if it has less than two items. I also removed the type check because reports never get created as Nones in the build() method. Instead, I added a test to verify that when calling build with report=None, the method still returns a dict. This PR also adds 3 tests for merge(). In the end, the added tests are the main contribution here.

[wholmgren]

thanks @alexandermorgan! @cwhanse ok with me to merge...