Fixing the issue where right column and top row generate wrong stream… #11455

pmackenz · 2018-06-18T17:57:35Z

The interpolation function in streamplot was duplicating the second to last right data column instead of using the rightmost column. Same issue for topmost row being replaced by the row just below the top.

PR Summary

This was indeed an indexing error. Fixing the index solved the issue.

Here the corrected images (same code as in the bug report, but now generating correct figures with streamlines as tangent to velocity vectors):

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

…lines

ImportanceOfBeingErnest · 2018-06-18T18:07:26Z

xref: This supposedly fixes #11452

jklymak · 2018-06-18T21:16:13Z

Thanks a lot @pmackenz ! You were right, it was an indexing issue. Thats great!

This fails all the image tests that used streamplot because now the streamplots are corrected. Someone will have to generate the new image comparison files and commit them as part of this PR before it can go in. Are you up to try? If not, I'm happy to help by pushing to your commit.

https://matplotlib.org/devel/testing.html

pmackenz · 2018-06-18T21:25:11Z

@jklymak , I am glad it was such a small issue to fix. However, I am really busy with work and don't have more time to donate for a few weeks. I'd appreciate if somebody else could update the test images.
Hint: overlay quiver with the same data. A critical visual check identifies issues easily. Courser grids are more obvious;)

jklymak · 2018-06-18T22:42:47Z

Ooookay, I think I did that right. Note I pushed onto yout matplotlib/master branch which is what you made this commit on. In the future, better to create a new branch in your repo rather than use "master"

jklymak · 2018-06-18T22:44:21Z

Reviewers, note that I updated the style and removed the text from all the tests...

dopplershift · 2018-06-18T22:52:50Z

Anyone else not getting the option to view image diffs?

jklymak · 2018-06-18T22:58:09Z

dopplershift · 2018-06-18T23:33:10Z

I swear that wasn't there earlier. 🐑

jklymak · 2018-06-18T23:33:22Z

Grrrr, I can't reproduce the ~~two~~ remaining image failures locally.

jklymak · 2018-06-19T04:39:33Z

Can anyone closer to the Travis build reproduce the error in pytest --pyargs matplotlib.tests.test_transforms::test_pre_transform_plotting? It runs fine on both of my machines, and is the only test failing, so I don't think its a setup problem.

jklymak · 2018-06-19T04:58:07Z

We may want to investigate what was meant here...

5632418

jklymak · 2018-06-19T16:45:02Z

OK think I fixed it. A bit annoying. Running

py.test -v --pep8 --pyargs matplotlib.tests.test_transforms::test_pre_transform_plotting

gives a warning:

/Users/jklymak/matplotlib/lib/matplotlib/testing/conftest.py:11: UserWarning:
This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.

and I couldn't get the images to match.

Calling as

py.test -v lib/matplotlib/tests/test_transforms.py

I don't get that warning and the images come out different.

Full warning stack

$ py.test -v --pep8 --pyargs matplotlib.tests.test_transforms::test_pre_transform_plotting

============================================ test session starts ============================================
platform darwin -- Python 3.6.3, pytest-3.3.1, py-1.5.2, pluggy-0.6.0 -- /Users/jklymak/anaconda3/envs/matplotlibdev/bin/python
cachedir: .cache
rootdir: /Users/jklymak/matplotlib, inifile: pytest.ini
plugins: pep8-1.0.6
/Users/jklymak/matplotlib/lib/matplotlib/testing/decorators.py:407: MatplotlibDeprecationWarning: The ImageComparisonTest class was deprecated in Matplotlib 3.0 and will be removed in 3.2.
  savefig_kwargs=savefig_kwarg, style=style)
/Users/jklymak/matplotlib/lib/matplotlib/testing/conftest.py:11: UserWarning:
This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.

The backend was *originally* set to 'Qt5Agg' by the following code:
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/bin/py.test", line 6, in <module>
    sys.exit(py.test.main())
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/config.py", line 59, in main
    return config.hook.pytest_cmdline_main(config=config)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 617, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
    res = hook_impl.function(*args)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 134, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 103, in wrap_session
    session.exitstatus = doit(config, session) or 0
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 140, in _main
    config.hook.pytest_collection(session=session)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 617, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
    res = hook_impl.function(*args)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 150, in pytest_collection
    return session.perform_collect()
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 640, in perform_collect
    items = self._perform_collect(args, genitems)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 662, in _perform_collect
    rep = collect_one_node(self)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/runner.py", line 502, in collect_one_node
    rep = ihook.pytest_make_collect_report(collector=collector)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 617, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
    res = hook_impl.function(*args)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/runner.py", line 369, in pytest_make_collect_report
    'collect')
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/runner.py", line 189, in __init__
    self.result = func()
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/runner.py", line 368, in <lambda>
    lambda: list(collector.collect()),
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 686, in collect
    for x in self._collect(arg):
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 696, in _collect
    names = self._parsearg(arg)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 753, in _parsearg
    parts[0] = self._tryconvertpyarg(parts[0])
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/main.py", line 733, in _tryconvertpyarg
    loader = pkgutil.find_loader(x)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/pkgutil.py", line 490, in find_loader
    spec = importlib.util.find_spec(fullname)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/importlib/util.py", line 88, in find_spec
    parent = __import__(parent_name, fromlist=['__path__'])
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.6/site-packages/_pytest/assertion/rewrite.py", line 212, in load_module
    py.builtin.exec_(co, mod.__dict__)
  File "/Users/jklymak/matplotlib/lib/matplotlib/tests/test_transforms.py", line 8, in <module>
    import matplotlib.pyplot as plt
  File "/Users/jklymak/matplotlib/lib/matplotlib/pyplot.py", line 68, in <module>
    from matplotlib.backends import pylab_setup
  File "/Users/jklymak/matplotlib/lib/matplotlib/backends/__init__.py", line 12, in <module>
    line for line in traceback.format_stack()


  matplotlib.use('agg')

jklymak · 2018-06-19T17:10:04Z

Milestoning for 3.0 - not a reversion, but certainly a bug that should be fixed sooner than later. I can't review because I pushed the images...

pmackenz · 2018-06-19T21:18:24Z

Wow, never expected to cause that much trouble with by two-character bug fix (really, just 2 characters). I very much appreciate how everybody is helping out here. This was my first micro-contribution aside from bug reports and I feel I owe everyone here! THANKS.

ImportanceOfBeingErnest · 2018-06-19T21:32:58Z

I guess the open question is (as mentionned above) still why that 2 instead of 1 got introduced in the first place.

pmackenz · 2018-06-19T21:40:19Z

Might have been a workaround for a different (now historic) bug. Workaround turned bug once the original issue was resolved ... ? OR: never been scrutinized by a picky user before. Wouldn't have noticed if I hadn't used streamplot to check a boundary condition on the right edge and after a few days of debugging concluded it wasn't my code doing this mistake.

timhoffm · 2018-06-23T12:45:58Z

The code was introduced in 5632418 as part of #664. That was quite a large merge and might really have got in without thoroughly thinking about the index.

The proposed change Nx - 2 -> Nx - 1 seems correct. However, I think the context of the code itself is flawed, or at least unnecessarily complex:

def interpgrid(a, xi, yi):
    """Fast 2D, linear interpolation on an integer grid"""

    Ny, Nx = np.shape(a)
    if isinstance(xi, np.ndarray):
        x = xi.astype(int)
        y = yi.astype(int)
        # Check that xn, yn don't exceed max index
        xn = np.clip(x + 1, 0, Nx - 1)
        yn = np.clip(y + 1, 0, Ny - 1)
    else:
        x = int(xi)
        y = int(yi)
        # conditional is faster than clipping for integers
        if x == (Nx - 2):
            xn = x
        else:
            xn = x + 1
        if y == (Ny - 2):
            yn = y
        else:
            yn = y + 1

    ...

Basically, what this does is for xi (which possibly may be a float or a np.array):

convert xi to integer -> x
create xn = x + 1 but make sure, it's not larger than the max index of a (Nx-1)

As far as I can see, the else clause is only relevant for single numbers.

Special handling single numbers seems unnecessary to me, you could just x = np.asarray(xi, dtype=int) and discard the whole else branch.
The comment "conditional is faster than clipping for integers" is hinting at an attempt of unnecessary optimization: Performance on single integers can only be a concern if you're calling that function many times. However then, you should call it once with an array instead.
Additionally, the two branches behave differently. While the if-branch clips, the else-branch just doesn't do the +1 shift for a single number. This is only the same as long as x is in the range (-1, Nx-1). Maybe the internal calls ensure that, but it's a fragile API.

I propose to replace the whole if-else block by

x = np.asarray(xi, dtype=int)
y = np.asarray(yi, dtype=int)
# Check that xn, yn don't exceed max index
xn = np.clip(x + 1, 0, Nx - 1)
yn = np.clip(y + 1, 0, Ny - 1)

dstansby

Thanks for the fix! This is going to need a changelog entry in doc/users/next_whats_new, since it is technically an API breaking change. Let us know if you have any questions about adding one.

jklymak · 2018-06-24T20:57:36Z

@dstansby done

@timhoffm I agree the code is suboptimal as it stands. OTOH, lets get this in, and then we can fix the underlying algorithm

dstansby · 2018-09-16T18:30:18Z

Does anyone have any thoughts on backporting to 2.2.x? It fixes a pretty major bug (see the above issue)

dstansby · 2018-09-25T08:59:47Z

@meeseekesbot backport to v2.2.x

This is due to an indexing error in previous versions, cf. matplotlib/matplotlib#11455.

QuLogic · 2018-09-26T03:47:54Z

It is a sort of bugfix, but it does cause downstream effects, e.g., SciTools/cartopy@1bf2c97 So if we do backport, it'd be good to know to get that condition right.

jklymak · 2018-10-21T00:52:46Z

@meeseeksdev backport to v2.2.x

lumberbot-app · 2018-10-21T00:52:54Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v2.2.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 f256b7958c1c58814be924d4b307e7a1cf91ac2a

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #11455: Fixing the issue where right column and top row generate wrong stream…'

Push to a named branch :

git push YOURFORK v2.2.x:auto-backport-of-pr-11455-on-v2.2.x

Create a PR against branch v2.2.x, I would have named this PR:

"Backport PR #11455 on branch v2.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

… top row generate wrong stream…

This is due to an indexing error in previous versions, cf. matplotlib/matplotlib#11455.

Fixing the issue where right column and top row generate wrong stream…

0f36b56

…lines

ImportanceOfBeingErnest mentioned this pull request Jun 18, 2018

Streamplot ignores rightmost column and topmost row of velocity data #11452

Closed

jklymak force-pushed the master branch from 59e2d36 to 2b6ad95 Compare June 18, 2018 22:43

dopplershift approved these changes Jun 18, 2018

View reviewed changes

jklymak force-pushed the master branch from d29582c to c1037e0 Compare June 19, 2018 02:28

TST: Fix streamplot test images

15fc856

jklymak force-pushed the master branch from c1037e0 to 15fc856 Compare June 19, 2018 16:37

TST: Fix streamplot test images

624ba7a

jklymak added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: needs review labels Jun 19, 2018

jklymak added this to the v3.0 milestone Jun 19, 2018

dstansby requested changes Jun 24, 2018

View reviewed changes

< 628C /a>

DOC: add API note

4af7f5e

dstansby approved these changes Jun 25, 2018

View reviewed changes

efiring merged commit f256b79 into matplotlib:master Jul 3, 2018

QuLogic removed the status: needs review label Jul 3, 2018

dstansby mentioned this pull request Sep 16, 2018

Streamplot does not work for 29x29 grid #12133

Closed

QuLogic added a commit to QuLogic/cartopy that referenced this pull request Sep 26, 2018

TST: Update streamplot result for MPL 3.0.0.

1bf2c97

This is due to an indexing error in previous versions, cf. matplotlib/matplotlib#11455.

lumberbot-app bot added the status: needs manual backport label Oct 21, 2018

timhoffm pushed a commit to timhoffm/matplotlib that referenced this pull request Oct 21, 2018

Backport PR matplotlib#11455: Fixing the issue where right column and…

7bd24c5

… top row generate wrong stream…

timhoffm mentioned this pull request Oct 21, 2018

Backport PR #11455 on branch v2.2.x #12579

Closed

timhoffm removed the status: needs manual backport label Oct 21, 2018

QuLogic added a commit to QuLogic/cartopy that referenced this pull request Nov 8, 2018

TST: Update streamplot result for MPL 3.0.0.

78424ec

This is due to an indexing error in previous versions, cf. matplotlib/matplotlib#11455.

QuLogic added a commit to QuLogic/cartopy that referenced this pull request Nov 8, 2018

TST: Update streamplot result for MPL 3.0.0.

d5282b9

This is due to an indexing error in previous versions, cf. matplotlib/matplotlib#11455.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fixing the issue where right column and top row generate wrong stream… #11455

Fixing the issue where right column and top row generate wrong stream… #11455

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fixing the issue where right column and top row generate wrong stream… #11455

Fixing the issue where right column and top row generate wrong stream… #11455

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!