Open fs446 opened 4 years ago
I like the PEP8 changes, except for the spaces around **
.
In the meantime, using
einsum
in this PR is definitely an improvement for the code.
"definitely" is quite a strong word. "improvement", too.
What if it's much slower?
I guess some instances can be replaced with the @
operator (a.k.a. __matmul__
). Have you checked the performance of that?
The test for Python 3.5 fails as the installed sphinxcontrib-bibtex
package no longer supports Python 3.5:
Exception occurred:
899 File "/home/travis/virtualenv/python3.5.7/lib/python3.5/site-packages/sphinxcontrib/bibtex/roles.py", line 12, in <module>
900 import pybtex.database
901 File "/home/travis/virtualenv/python3.5.7/lib/python3.5/site-packages/pybtex/database/__init__.py", line 124
902 f" entries={repr_entry},\n\n"
903 ^
904SyntaxError: invalid syntax
I also very much dislike Python 3.5 and would simply remove support for it.
The other test fails as homebrew
seems not to be installing pandoc
I created a pull request where I run the tests directly on Github (and skip Python 3.5): https://github.com/sfstoolbox/sfs-python/pull/166 There the MacOS tests work as well. So it just seems to be a bug in how travis is configured.
I rebased the master, now the tests are passing.
I rebased and made code work for recent 0.6.1 / 0.6.2. We will need a handling once inner1d() is discontinued, so it might be a good idea to keep this einsum() handling up to date. I would vote using this some time.
We use
from numpy.core.umath_tests import inner1d as _inner1d
which is suboptimal, since its deprecated and requires non-standard library import. See also https://github.com/sfstoolbox/sfs-python/issues/77We could use numpy's
einsum
do to this job instead, thus I've changed (hopefully) all occurrences ofinner1d
toeinsum
.Besides that in the files, some PEP8 modifications were done, these suggestions came from PyCharm.
Maybe numpy's
tensordot
is even more performant/suitable. I don't know currently how/if these functions support parallel computation, whichinner1d
seemed to do and was chosen for. I will check this.In the meantime, using
einsum
in this PR is definitely an improvement for the code.