codeforboston / home-energy-analysis-tool

https://www.codeforboston.org/projects/
MIT License
12 stars 30 forks source link

Optimize billing period - temperature merging #114

Closed jkwan2011 closed 10 months ago

jkwan2011 commented 10 months ago

The list of billing periods is built by looking up indices in the temperature input and creating a list of temperatures in a for loop. Assuming the temperature input is sorted by date, consider using a more optimal method of generating this list. @debajyotid2 has suggested using the bisect module.

Note that this in reference to lines 64-67 of engine.py for the latest commit on the summary-outputs branch.

debajyotid2 commented 10 months ago

@jkwan2011 I made the changes. (The original lines are commented out) Please review here: https://github.com/codeforboston/home-energy-analysis-tool/commit/be7987a5f40e32f738b3bc4cf36a04f340a541b8 .

If the changes look good please close this issue as completed.

Edit: Reverted an unintended change that I accidentally left in. It has been fixed now here: https://github.com/codeforboston/home-energy-analysis-tool/commit/6a46074a1a0cf50584d273fd51a01bfb062e9753.

jkwan2011 commented 10 months ago

Thanks for the contribution! It looks good, but for the end_idx, is there a reason to using bisect_left + 1, instead of bisect_right?

debajyotid2 commented 10 months ago

Thanks for the contribution! It looks good, but for the end_idx, is there a reason to using bisect_left + 1, instead of bisect_right?

Thanks! The difference between bisect_left and bisect_right is that if there are repeated values of an element x that you want to insert in an array, bisect_left will return the left-most index where it can be inserted to keep the array sorted, while bisect_right will return the right-most index. In this case I assumed there would be no repeated dates, so there would be no difference in start_idx and end_idx if you used either of the two functions. The 1 is added as for the slice based indexing we want to be including temperatures from start_idx to end_idx inclusive of both ends. (Since the billing period start date and end date are both considered inclusive when searching for temperature values within that period).

jkwan2011 commented 10 months ago

Thanks for the explanation!