-
Notifications
You must be signed in to change notification settings - Fork 998
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.
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
refactor bifacial merge, improve merge tests #747
Changes from 10 commits
0d4cb80
55d8f08
c969994
93d0559
e244343
84d7189
d131657
94f2db5
4fdb599
d1429d5
e12b807
25da151
feb7bf3
079bb48
2189952
39c2889
eeb8053
465d228
c108362
4447e6c
74c8eba
4eaf93f
b2ed214
c2fbecb
fa9b64a
be8ec1c
80809f2
797dc83
bd0740a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
|
||
Contributors | ||
~~~~~~~~~~~~ | ||
* Alexander Morgan (:ghuser:`alexandermorgan`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,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()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not at all familiar with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you provide a test please? Also is it possible to use >>> 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since all reports are currently initialized with this (line 162)
the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the gain in performance in this case? |
||
return report |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.