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

Fix NumpyVectorArray.__mul__ when other is a NumPy array #824

Merged
merged 4 commits into from Dec 12, 2019

Conversation

pmli
Copy link
Member

@pmli pmli commented Dec 9, 2019

No description provided.

@pmli pmli added this to the 2019.2 milestone Dec 9, 2019
@pmli pmli requested a review from sdrave Dec 9, 2019
@pmli
Copy link
Member Author

@pmli pmli commented Dec 11, 2019

There is still the issue with __rmul__. It seems that in array * vectorarray, np.ndarray.__mul__ wants to iterate over vectorarray, which raises AssertionError when the index is out of bounds.

>>> import numpy as np
>>> from pymor.vectorarrays.numpy import NumpyVectorSpace
>>> a = np.array([1, 2])
>>> v = NumpyVectorSpace(3).ones(2)
>>> v * a
NumpyVectorArray(
    [[1. 1. 1.]
     [2. 2. 2.]],
    NumpyVectorSpace(3))
>>> a * v
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/.../src/pymor/vectorarrays/numpy.py", line 69, in __getitem__
    return NumpyVectorArrayView(self, ind)
  File "/.../src/pymor/vectorarrays/numpy.py", line 446, in __init__
    assert array.check_ind(ind)
AssertionError

@sdrave
Copy link
Member

@sdrave sdrave commented Dec 11, 2019

@pmli, fixing this will probably require some thought: we cannot prevent ndarray.__mul__ from being called first. Probably we would have to raise IndexError at the very least (no idea what will happen then). I am a bit hesitant to do so, however, since I am not sure how expensive check_ind is and I would prefer not having to call it when running with -O. Maybe it's ok to accept this issue for the release?

@pmli
Copy link
Member Author

@pmli pmli commented Dec 11, 2019

I'm fine with postponing this particular issue (I'm also not sure what is the best approach). I removed the failing test.

@codecov
Copy link

@codecov codecov bot commented Dec 11, 2019

Codecov Report

Merging #824 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted Files Coverage Δ
src/pymor/vectorarrays/numpy.py 83.37% <50%> (-0.68%) ⬇️

@sdrave sdrave merged commit 191d361 into master Dec 12, 2019
8 checks passed
@sdrave sdrave deleted the np-va-mul-array branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants