Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace inner1d bei einsum #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace inner1d bei einsum #163

wants to merge 1 commit into from

Conversation

fs446
Copy link
Member

@fs446 fs446 commented Mar 11, 2020

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 #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 fs446 requested a review from mgeier March 11, 2020 13:15
@mgeier
Copy link
Member

mgeier commented Mar 12, 2020

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
Copy link
Member

mgeier commented Mar 17, 2020

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

@hagenw
Copy link
Member

hagenw commented Nov 27, 2020

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
Copy link
Member

hagenw commented Nov 27, 2020

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

@hagenw
Copy link
Member

hagenw commented Nov 27, 2020

I created a pull request where I run the tests directly on Github (and skip Python 3.5): #166
There the MacOS tests work as well. So it just seems to be a bug in how travis is configured.

@hagenw
Copy link
Member

hagenw commented Dec 4, 2020

I rebased the master, now the tests are passing.

@fs446 fs446 changed the title Update wfs.py in fd/td, util.py Replace inner1d bei einsum Jun 6, 2021
@fs446
Copy link
Member Author

fs446 commented Jun 6, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants