Skip to content

refactor bifacial merge, improve merge tests #747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wholmgren merged 29 commits into from
Jul 20, 2019
Merged

refactor bifacial merge, improve merge tests #747

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0d4cb80
Simplify merge method.
alexandermorgan Jul 8, 2019
55d8f08
Update what's new doc.
alexandermorgan Jul 8, 2019
c969994
Update whatsnew file syntax.
alexandermorgan Jul 8, 2019
93d0559
Rename whatsnew file to change format to rst.
alexandermorgan Jul 8, 2019
e244343
Fix linting errors.
alexandermorgan Jul 8, 2019
84d7189
Remove pandas and numpy version reqs
alexandermorgan Jul 9, 2019
d131657
Update v0.7.1.rst
alexandermorgan Jul 9, 2019
94f2db5
Revert "Update v0.7.1.rst"
alexandermorgan Jul 9, 2019
4fdb599
Revert "Remove pandas and numpy version reqs"
alexandermorgan Jul 9, 2019
d1429d5
Merge branch 'master' into bifacial_dicts
alexandermorgan Jul 9, 2019
e12b807
Reword elements in merge() comprehension.
alexandermorgan Jul 10, 2019
25da151
Fix handling of None values in merge.
alexandermorgan Jul 10, 2019
feb7bf3
Remove unnecessary checks in merge().
alexandermorgan Jul 11, 2019
079bb48
Add tests for merge() and build().
alexandermorgan Jul 11, 2019
2189952
Fix lint errors.
alexandermorgan Jul 11, 2019
39c2889
Correct import mistake.
alexandermorgan Jul 12, 2019
eeb8053
Reinsert type check in merge loop.
alexandermorgan Jul 12, 2019
465d228
Revert "Reinsert type check in merge loop."
alexandermorgan Jul 12, 2019
c108362
Fix test_build_1 test.
alexandermorgan Jul 12, 2019
4447e6c
Update what's new file.
alexandermorgan Jul 12, 2019
74c8eba
Fix merge conflicts in what's new file.
alexandermorgan Jul 12, 2019
4eaf93f
Revert "Fix merge conflicts in what's new file."
alexandermorgan Jul 13, 2019
b2ed214
Revert "Update what's new file."
alexandermorgan Jul 13, 2019
c2fbecb
Revert "Rename whatsnew file to change format to rst."
alexandermorgan Jul 13, 2019
fa9b64a
Revert "Update whatsnew file syntax."
alexandermorgan Jul 13, 2019
be8ec1c
Revert "Update what's new doc."
alexandermorgan Jul 13, 2019
80809f2
Merge what's new updates from upstream.
alexandermorgan Jul 13, 2019
797dc83
Update what's new file.
alexandermorgan Jul 13, 2019
bd0740a
Merge branch 'master' into bifacial_dicts
alexandermorgan Jul 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/sphinx/source/whatsnew/v0.7.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.. _whatsnew_0710:

v0.7.1 (JULY 8, 2019)
---------------------

This is a minor release that does the following:

**Simplify and optimize the performance the merge method in bifacial.py
Copy link
Member

@wholmgren wholmgren Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in 0.7.0, right?

Copy link
Contributor Author

@alexandermorgan alexandermorgan Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@alexandermorgan alexandermorgan Jul 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


Contributors
~~~~~~~~~~~~
* Alexander Morgan (:ghuser:`alexandermorgan`)
19 changes: 8 additions & 11 deletions pvlib/bifacial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -177,13 +176,11 @@ def build(report, pvarray):

@staticmethod
def merge(reports):
"""Works for dictionary reports"""
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]
"""Works for dictionary reports. Merges the reports list of
dictionaries by flattening the lists for each key into a single
super list. Returns a dictionary with two list values."""
# Dictionary comprehension obviates the need to check if there are more
# than one report, and if one of the elements in reports is None.
report = {k: [item for d in reports for item in d[k]]
for k in reports[0].keys()}
Copy link
Member

@cwhanse cwhanse Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@alexandermorgan alexandermorgan Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@wholmgren wholmgren Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@mikofski mikofski Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@anomam anomam Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@wholmgren wholmgren Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the gain in performance in this case?

return report