Closed vonDonnerstein closed 3 years ago
Hello @vonDonnerstein! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
quimb/tensor/optimize.py
:Line 564:80: E501 line too long (82 > 79 characters) Line 597:80: E501 line too long (83 > 79 characters) Line 597:84: W291 trailing whitespace Line 598:80: E501 line too long (84 > 79 characters) Line 652:34: E231 missing whitespace after ',' Line 655:14: W291 trailing whitespace Line 674:39: E231 missing whitespace after ',' Line 683:80: E501 line too long (86 > 79 characters) Line 685:33: E231 missing whitespace after ',' Line 685:41: E231 missing whitespace after ',' Line 685:49: E231 missing whitespace after ',' Line 690:80: E501 line too long (134 > 79 characters) Line 691:80: E501 line too long (119 > 79 characters) Line 692:47: E231 missing whitespace after ',' Line 692:71: E231 missing whitespace after ',' Line 692:80: E501 line too long (111 > 79 characters) Line 692:92: E231 missing whitespace after ',' Line 700:43: E231 missing whitespace after ',' Line 704:18: E261 at least two spaces before inline comment Line 708:49: E261 at least two spaces before inline comment Line 708:80: E501 line too long (109 > 79 characters) Line 708:110: W291 trailing whitespace Line 709:50: E114 indentation is not a multiple of four (comment) Line 709:50: E117 over-indented (comment) Line 709:80: E501 line too long (117 > 79 characters) Line 714:36: E221 multiple spaces before operator Line 719:77: E231 missing whitespace after ',' Line 719:80: E501 line too long (113 > 79 characters) Line 725:36: E221 multiple spaces before operator Line 782:80: E501 line too long (94 > 79 characters)
Hey sorry it took me a while to get round to this. I had a lot of changes in parallel to optimize.py
so in the end I have incorporated those and pushed to your branch so as to still acknowledge you in the commit history. Next time hopefully will be able to more of a review and revise.
The summary of changes are:
as_completed
call as to at least a first approx summing the gradients is probably a negligible cost compared to computing gradientsTODO maybes:
as_completed
if needed performance wiseUnless there are any comments, I'm just going to look into the failing tests and then merge.
see discussion on slack