Fix issue #6003 #6011

Merged
merged 6 commits into from Mar 20, 2016

Conversation

Projects
None yet
5 participants

#6003

Instreamplot, skip start_points that are on an existing streamline instead of raising InvalidIndexError.

mdboom added the needs_review label Feb 16, 2016

Member

WeatherGod commented Feb 16, 2016

Should add a unit test to exercise this case.

On Tue, Feb 16, 2016 at 5:49 PM, muahah notifications@github.com wrote:

In streamplot, skip start_points that are on an existing streamline

instead of raising InvalidIndexError.

You can view, comment on, or merge this pull request online at:

#6011
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#6011.

Run into another problem while adding a test case :
streamplot() accept 1d or 2d arrays for x and y arguments.
But trying to use 2d arrays while specifying start_points lead to an error.
At a first though, transforming x and y to 1d arrays at streamplot.py:140 should fix the problem.
I will try to do so soon.

tacaswell added this to the 2.1 (next point release) milestone Feb 17, 2016

Owner

tacaswell commented Feb 19, 2016

attn @tonysyu

I fixed issue #6003 and added a test case for streamplot() with non-null start_points argument.
The test case raised an error because of the use of 2-dimensionnal arrays for x and y (Grid class, that normally deal with 2-dimensionnal arrays is not called in that case), so I made some changes for that too.

@QuLogic QuLogic commented on an outdated diff Feb 29, 2016

lib/matplotlib/streamplot.py
@@ -137,8 +137,14 @@ def streamplot(axes, x, y, u, v, density=1, linewidth=None, color=None,
# Shift the seed points from the bottom left of the data so that
# data2grid works properly.
sp2 = np.asanyarray(start_points, dtype=np.float).copy()
- sp2[:, 0] += np.abs(x[0])
- sp2[:, 1] += np.abs(y[0])
+ if x.ndim == 1 and y.ndim == 1:
+ sp2[:, 0] += np.abs(x[0])
+ sp2[:, 1] += np.abs(y[0])
+ elif x.ndim == 2 and y.ndim == 2:
+ sp2[:, 0] += np.abs(x[0][0])
+ sp2[:, 1] += np.abs(y[0][0])
+ else:
+ raise ValueError("'x' and 'y' should have the same dimensions")
@QuLogic

QuLogic Feb 29, 2016

Member

This message is not strictly correct if, e.g., x.ndim == y.ndim == 3.

By looking more closely, those checks are redundant because Grid already checks x and y (ndim and values).
Grid also extract x and y origins that can be re-used to shift the seed points, instead of getting them from data.

Member

QuLogic commented Mar 2, 2016

So is this complete? None of the streamlines in the test image appear to originate at the plotted points.

Also, the file names do not need "test_image" in them; it's clear it's a test image from the directory.

galaunay commented Mar 2, 2016

I pointed out the fact that streamlines do not exactly originate from starting points in #6002.
Don't know yet if particular seeding positions on the grid are needed by the streamline algorithm or if there is just a problem while passing between data coordinates and grid coordinates.
I will keep looking on that.
I don't know if it belongs to another pull request or not.

Regarding the image names, I can change them.

Owner

tacaswell commented Mar 7, 2016

Please do not merge the master branch into your branch. Instead rebase your branch on top of current master and then force-push to your gh repo.

galaunay commented Mar 7, 2016

I realize that this pull request is a bit messy (I'm slowly getting used to git and github).

Does it sound good to you if we close it so I can open a new (clean) pull request for issues #6002 and #6003 when it will be ready ?

Owner

tacaswell commented Mar 7, 2016

The easiest thing to do is to fix this is to rebase + force push. I just did the rebase locally and there is fortunately no conflicts so (I am going to assume that your upsrteam remote is called 'matplotlib' and the remote pointing at your gh account is called origin

# update your computer to know what gh knows
git remote update  
# make sure you are one the branch this PR looks at
git checkout master
# rebase onto current master
git rebase matplotlib/master
# check gitk to make sure tis went well

# fail to push to your gh for pedagogical reasons 
git push origin master:master
# try again with --force, only use this when you are sure
git push --force origin master:master

Thank you, this is better now.
I think this is it, I had to change the way the function pass from data coordinates to internal grid coordinates (the bug was here).
After that some tests failed with low RMS (0.001 and 0.072).
Theoretically, the change made should not modify the result from streamplot(start_points=None) but it indeed change the way data are passed from and to grid coordinates. So I assume small differences in tests are due to numerical errors...

Owner

tacaswell commented Mar 19, 2016

It looks like you added some new test images and then modified them in later commits. Can you squash these down into a single commit so that the repository does not carry around the (wrong) intermediary results?

muahah added some commits Mar 19, 2016

muahah Fix issue #6003
Starting points on the same streamline don't raise IndexInvalidError anymore.
e9ddd00
muahah Fix starting points misplacement
Fix passage from data to grid coordinates for starting points.
71b9ae8
muahah Add some tests to prevent invalid starting points
Raise an explicit error when specified starting points are outside of
the data boundaries.
5a27f80
muahah Rename 'streamplot' test cases bd63121
muahah Add a new test case for 'streamplot'
With specified 'start_points'.
20959b3
muahah Update a failed test case
Change in 'streamplot' slightly modified some test cases
875b936

I rebased the whole thing to have clean an explicit commits (and no intermediary results).

Owner

tacaswell commented Mar 19, 2016

Thanks!

The lower left point in the start point test does fall exactly on the stream line. Is that expected?

The streamline do not originate from this point but from the point in the very corner (-3; -3).
the point at (-2.33, -2.33) is ignored because too close to an existing streamline (c.f. e9ddd00).
It is possible to get it back by increasing the density parameter.

Owner

tacaswell commented Mar 20, 2016

Ah, I missed that point was there.

@tacaswell tacaswell added a commit that referenced this pull request Mar 20, 2016

@tacaswell tacaswell Merge pull request #6011 from muahah/master
Fix #6003
9110be0

@tacaswell tacaswell merged commit 9110be0 into matplotlib:master Mar 20, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment