sfs-python icon indicating copy to clipboard operation
sfs-python copied to clipboard

Replace inner1d bei einsum

Open fs446 opened this issue 5 years ago • 8 comments

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/77

We could use numpy's einsum do to this job instead, thus I've changed (hopefully) all occurrences of inner1d to einsum.

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, which inner1d 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.

fs446 avatar Mar 11 '20 13:03 fs446

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?

mgeier avatar Mar 12 '20 13:03 mgeier

I guess some instances can be replaced with the @ operator (a.k.a. __matmul__). Have you checked the performance of that?

mgeier avatar Mar 17 '20 18:03 mgeier

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.

hagenw avatar Nov 27 '20 07:11 hagenw

The other test fails as homebrew seems not to be installing pandoc

hagenw avatar Nov 27 '20 07:11 hagenw

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.

hagenw avatar Nov 27 '20 07:11 hagenw

I rebased the master, now the tests are passing.

hagenw avatar Dec 04 '20 22:12 hagenw

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.

fs446 avatar Jun 06 '21 12:06 fs446