Closed adamcallison closed 4 years ago
Hey Adam, sorry just very busy with non-work stuff the next few days but this looks nice and I'll try and get to it soon.
Just one brief comment is that python has the inspect
module for working out how many arguments a function takes - might simplify things?
Hi, Johnnie - no rush, good luck with the next few days :)
I considered going via inspect
, but I ultimately decided on try except
because I didn't want to cause problems for more general callables that inspect
might not like. Are you concerned that my approach might be slowed down the try except
block?
I will leave this PR as is for now, happy to pick this discussion back up when things settle down for you :)
Hey Adam, so after much wrestling I seem to have brought quimb+tests+travis into harmony again. If you are still interested in this, could you rebase onto/merge the new changes?
Hello @adamcallison! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
Merging #49 into develop will increase coverage by
4.19%
. The diff coverage is97.22%
.
@@ Coverage Diff @@
## develop #49 +/- ##
===========================================
+ Coverage 84.88% 89.08% +4.19%
===========================================
Files 33 33
Lines 8615 8627 +12
===========================================
+ Hits 7313 7685 +372
+ Misses 1302 942 -360
Impacted Files | Coverage Δ | |
---|---|---|
quimb/evo.py | 98.63% <97.22%> (-0.41%) |
:arrow_down: |
quimb/tensor/tensor_core.py | 91.44% <0%> (+0.3%) |
:arrow_up: |
quimb/linalg/base_linalg.py | 90.64% <0%> (+0.58%) |
:arrow_up: |
quimb/tensor/array_ops.py | 82.43% <0%> (+1.35%) |
:arrow_up: |
quimb/tensor/optimize_autograd.py | 87.43% <0%> (+50.26%) |
:arrow_up: |
quimb/tensor/optimize_tensorflow.py | 67.92% <0%> (+67.92%) |
:arrow_up: |
quimb/tensor/optimize_pytorch.py | 73.37% <0%> (+73.37%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2bcdd4a...f3ef77b. Read the comment docs.
Hey Adam, so after much wrestling I seem to have brought quimb+tests+travis into harmony again. If you are still interested in this, could you rebase onto/merge the new changes?
Hi Johnnie, hope all is well. Rebase done (plus an extra commit to make it pep8 compliant)
OK, finally got round to reviewing this, all looks good! Thanks for the PEP8 updates and another all round good and thorough pull request.
Some repeated code in _setup_callbacks factored out in the process. Existing tests extended to cover these changes.