Add maximum streamline length property. by gzahl · Pull Request #6062 · matplotlib/matplotlib

gzahl · 2016-02-25T22:09:39Z

Hello,
I have added the option to specify the maximum streamline length. This is handy if one e.g. only needs a single (or a few) streamlines. It is especially handy if used together with "start_points".

This may be enhanced by forcing only forward (or backward) streamline integration. But that should probably be taken care of in another patch. Do you agree?

tacaswell · 2016-02-25T22:11:56Z

Can you add a test for this?

Other than that seems perfectly reasonable and I agree doing the integration direction as a second PR is a good idea. Smaller changes are easier to review.

gzahl · 2016-02-25T22:25:46Z

I will try to set up a short test.

My first interface idea for the integration direction is to change maxlength optionally in a tuple:
maxlength=[0,2] # this would be only forward integration.
maxlength=[-4,0] # only (longer) backward integration.
maxlength=2 # this would be translated to maxlength=[-2,2], which is the old default.

This would give the most flexibility. Or is it too complicated?

tacaswell · 2016-02-25T22:51:12Z

The negatives would be confusing. I think a tri-state integration_direction in {'forward', 'reverse', 'both'} and then the maximum_length is either all forward, all back or split evenly would be simpler and get almost as much power.

gzahl · 2016-02-25T23:49:45Z

Ok, i have added a test and copied the interface to axes and pyplot. I hope this was the right thing to do?
Tristate sounds good. Do you have an example, how it should be done in matplotlib?
I think 'backward' is better than 'reverse', since it is the standard term when referring to integrations.

tacaswell · 2016-02-25T23:51:36Z

I think just strings are good enough (just validate on the way in).

You will also need to commit the test image see http://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

This should be a png only test I think.

gzahl · 2016-02-26T00:58:14Z

Hi,
thanks for the all your help and quick replies! I now have added two more patches:

The baseline image for the maxlength test
A complete patch + test for the integration direction

Let me know, if anything has to be changed and i will try to do it tomorrow.

gzahl · 2016-02-26T11:55:58Z

Ok, after ironing out my rookie mistakes, i have added two more commits.
Now only a single test fails, but it does not seem to be realted to my commits.
Is there anything i can do? Thanks!

edit: Seems like the last problem fixed itself!

tacaswell · 2016-03-07T00:00:01Z

lib/matplotlib/axes/_axes.py

    def streamplot(self, x, y, u, v, density=1, linewidth=None, color=None,
                   cmap=None, norm=None, arrowsize=1, arrowstyle='-|>',
-                   minlength=0.1, transform=None, zorder=2, start_points=None):
+                   minlength=0.1, maxlength=4.0, transform=None, zorder=2,


All new args need to go at the end to maintain back-compatibility. The reason is if people are (un wisely!) passing in everything as positional args.

When we can drop python2 and can break API again most of these should be converted to kwarg only, but for now they need to go at the end.

tacaswell · 2016-03-07T00:06:21Z

Left a few minor comments that need to be addressed. I see what you are doing putting minlength and maxlength together, but we should not break API 😞 .

Could you also add an entry into https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new so that this gets properly advertised in the next release?

gzahl · 2016-03-14T20:53:34Z

Hi, Sorry for the delay. It seems like i don't get any Email notifications. I have hopefully addressed all the issues.
I will try to enable my notifications.

tacaswell · 2016-03-14T21:42:56Z

lib/matplotlib/streamplot.py

    *minlength* : float
        Minimum length of streamline in axes coordinates.
+    *maxlength* : float
+        Maximum length of streamline in axes coordinates.


can you re-arrange the docstrings too?

gzahl · 2016-11-03T16:14:53Z

The tests are failing after the merge with upstream. I have to track that down later.

afvincent · 2016-11-03T17:02:42Z

Might the test failures be related to the fact that FancyArrowPatch (which is used for streamplots) was modified in parallel with your PR (#6504 that superseded #6054)?

tacaswell · 2016-11-03T19:04:18Z

Sorry this got dropped!

Can you rebase instead of merging upstream into your branch?

See http://matplotlib.org/devdocs/devel/gitwash/development_workflow.html?highlight=rebase

gzahl · 2016-11-04T11:04:11Z

Thanks for the hint about a rebase rather than a merge.
But if i now revert the merge commit, i still have the merge/merge-revert in the history, which makes the rebase rather difficult (After conflicts with my changes, i get conflicts because of the merge commit).
Is there an easier way to do it? Sorry to bother you because of this. The easiest way would to make a new branch, and cherry-pick my changes. But i can not change the branch attached to this pull request.

WeatherGod · 2016-11-04T14:04:14Z

If you do an interactive rebase, you can delete the commits that you don't
want in the branch and then force-push that up to github.

On Fri, Nov 4, 2016 at 7:04 AM, Manuel Jung notifications@github.com
wrote:

Thanks for the hint about a rebase rather than a merge.
But if i now revert the merge commit, i still have the merge/merge-revert
in the history, which makes the rebase rather difficult (After conflicts
with my changes, i get conflicts because of the merge commit).
Is there an easier way to do it? Sorry to bother you because of this. The
easiest way would to make a new branch, and cherry-pick my changes. But i
can not change the branch attached to this pull request.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6062 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-AWfEMOjbTmqfAsmDjVSw-XVJLTsks5q6xEsgaJpZM4HjPdA
.

gzahl · 2016-11-04T14:52:44Z

Ok, thanks for the hint about the interactive rebase. The rebase is done, but the test (of course) still fail.
I have looked into it for a bit, but i don't think this is (mainly) because of the #6504. The starting/end points seem to be slightly different. I will look into this asap.

afvincent · 2016-11-04T16:31:50Z

If the diff images do not show any issue with the arrows (especially their size), then I think you are right about #6504 not being related to the test failures.

gzahl · 2016-11-07T13:15:14Z

Looking at the history of streamplot.py and its test file, i make the following observations:

The test cases have been renamed bd63121. I will do the same with mine, to comply to the others.
My observation about the different starting points seem to be related to commit 71b9ae8. Following this a new test case has been submitted 20959b3, which uses the starting_points option, which is also used by my tests. I therefore conclude, that this fix results in the changes of my test and will resubmit the expected image (Since it otherwise seems to be fine).

QuLogic · 2016-11-07T23:22:37Z

Can you rebase and remove the extra copies of the test images?

Do an interactive rebase, move the update commits right below the original addition of the files, and change the update commits to 'fixup'.

QuLogic

Some minor stuff + the rebase mentioned earlier.

QuLogic · 2016-11-07T23:23:19Z

doc/users/whats_new/streamplot_set_maximum_length.rst

+Maximum streamline length and integration direction can now be specified
+------------------------------------------------------------------------
+
+This allows to follow the vector field for a longer time and can enhance the visibilty of the flow pattern in some use cases.


visibilty -> visibility

Also, wrap at 80.

QuLogic · 2016-11-07T23:23:57Z

lib/matplotlib/pyplot.py

 # changes will be lost
 @_autogen_docstring(Axes.hist)
-def hist(x, bins=10, range=None, normed=False, weights=None, cumulative=False,
+def hist(x, bins=None, range=None, normed=False, weights=None, cumulative=False,


Unrelated change?

Yes, but this is generated by boilerplate.py. Maybe it was changed in another commit, but boilerplate.py was not run? I rerun boilerplate.py in the 9634c0e.

I think it is because pyplot was not updated with f1b89b2 change

That does seem right. Should this be fixed in another issue?

It's fine to update this here. We should be better about not letting pyplot get 'stale', but it is what it is.

Okay. I guess this request can therefore be closed.
I rebase again yesterday. Again there are unrelated changes from boilerplate.py (4775d7e
).

QuLogic · 2016-11-07T23:25:04Z

lib/matplotlib/streamplot.py

        any number
+    *maxlength* : float
+        Maximum length of streamline in axes coordinates.
+    *integration_direction* : ['foward','backward','both']


foward -> forward

space after commas

QuLogic · 2016-11-07T23:25:49Z

lib/matplotlib/streamplot.py


+    if integration_direction not in ['both', 'forward', 'backward']:
+        errstr = ("Integration direction '%s' not recognised." %
+          integration_direction)


Misaligned.

QuLogic · 2016-11-07T23:26:11Z

lib/matplotlib/streamplot.py

+    if integration_direction not in ['both', 'forward', 'backward']:
+        errstr = ("Integration direction '%s' not recognised." %
+          integration_direction)
+        errstr += "Expected 'both', 'forward' or 'backward'."


Why not place it all in one string from the onset?

QuLogic · 2016-11-07T23:27:30Z

lib/matplotlib/streamplot.py

+            dmap.reset_start_point(x0, y0)
+            s, xt, yt = _integrate_rk12(x0, y0, dmap, forward_time, maxlength)
+            if len(x_traj) > 0:
+              xt = xt[1:]


Indent should be a multiple of 4.

QuLogic · 2016-11-07T23:28:07Z

lib/matplotlib/tests/test_streamplot.py

+def swirl_velocity_field():
+    x = np.linspace(-3.,3.,100)
+    y = np.linspace(-3.,3.,100)
+    X,Y = np.meshgrid(x,y)


Add space after commas in above 3 lines.

QuLogic · 2016-11-07T23:28:40Z

lib/matplotlib/tests/test_streamplot.py

+def test_maxlength():
+    x, y, U, V = swirl_velocity_field()
+    plt.streamplot(x, y, U, V, maxlength=10., start_points=[[0.,1.5]],
+                   linewidth=2,density=2)


Add space after commas in above two lines.

QuLogic · 2016-11-07T23:28:53Z

lib/matplotlib/tests/test_streamplot.py

+def test_direction():
+    x, y, U, V = swirl_velocity_field()
+    plt.streamplot(x, y, U, V, integration_direction='backward',
+                   maxlength=1.5, start_points=[[1.5,0.]],


Space after comma.

# Das ist die erste Commit-Beschreibung: Test streamplot maxlength feature. # Commit-Beschreibung matplotlib#2 wird ausgelassen: # Added streamline maxlength test png baseline image.

gzahl · 2016-11-21T07:44:41Z

@QuLogic Github still shows me a requested change, but i think i did all revisions? Otherwise please leave me a hint what is missing. Thanks!

NelleV · 2016-11-21T21:27:06Z

lib/matplotlib/streamplot.py

        In data coordinates, the same as the ``x`` and ``y`` arrays.
    *zorder* : int
        any number
+    *maxlength* : float


Can you please remove the "*". They are not helpful for readability.

Of course i could, but all parameters in this function use the "*". It seems inconsistent if i only remove them for the new "maxlength" parameter?

NelleV · 2016-11-21T21:27:21Z

lib/matplotlib/streamplot.py

        any number
+    *maxlength* : float
+        Maximum length of streamline in axes coordinates.
+    *integration_direction* : ['forward', 'backward', 'both']


Can you specify the default in the docstring?

Oh, according to numpydoc, it should be braces {} instead of brackets [], then the first value is the default (so put 'both' first.)

NelleV · 2016-11-21T21:28:17Z

lib/matplotlib/streamplot.py

 #========================

-def get_integrator(u, v, dmap, minlength):
+def get_integrator(u, v, dmap, minlength, maxlength, integration_direction):


I don't think this needs to be public. Can you make this function private?

Ok, if it was public, i should leave it like that?

NelleV · 2016-11-21T21:30:52Z

Hi @gzahl
It looks good overall, but do you mind making that new function private? Else, it adds this to the list of elements that need to be backward compatible :)
The "*" removal is just bonus points: we can merge this PR without that nitpick being fixed.

QuLogic · 2016-11-21T21:32:06Z

Unfortunately, that function is not new (WRT this PR, at least.)

NelleV · 2016-11-21T21:36:19Z

Indeed, it is not… (I am clearly not caffeinated enough).

tacaswell · 2016-11-22T01:12:31Z

Glad this finally got merged!

@gzahl Thanks for your contribution and sorry the review took so long!

gzahl · 2016-11-22T08:15:39Z

Thanks @tacaswell and all the others for your patience!

mdboom added the status: needs review label Feb 25, 2016

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

tacaswell reviewed Mar 7, 2016
View reviewed changes

tacaswell reviewed Mar 14, 2016
View reviewed changes

gzahl force-pushed the master branch from 37d3d1f to bb6bfb1 Compare November 4, 2016 14:22

gzahl force-pushed the master branch from 8744758 to 3422bca Compare November 7, 2016 13:19

QuLogic requested changes Nov 7, 2016

View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Nov 8, 2016

tacaswell changed the title ~~Add maximum streamline length property.~~ [MRG+1] Add maximum streamline length property. Nov 9, 2016

tacaswell approved these changes Nov 9, 2016

View reviewed changes

Manuel Jung added 11 commits November 21, 2016 08:15

Add maximum streamline length property.

b14d46d

Copy streamplot interface addition to axes and pyplot interfaces.

c60bfe2

# Das ist eine Kombination aus 2 Commits.

67aa36e

# Das ist die erste Commit-Beschreibung: Test streamplot maxlength feature. # Commit-Beschreibung matplotlib#2 wird ausgelassen: # Added streamline maxlength test png baseline image.

Test streamplot maxlength feature.

ea75ec1

New streamplot option integration_direction added.

6cef6bc

Comply with pep8.

e67ade8

conserve kwargs order.

ac00e8d

Add Whatsnew entry for maxlength streamplot feature.

e3d24e8

rearranged docstrings.

6695ea9

Revision changes

c014846

Add missing whitespace

4652f7f

gzahl force-pushed the master branch from 4775d7e to 4652f7f Compare November 21, 2016 07:42

NelleV added status: needs review and removed status: needs revision labels Nov 21, 2016

NelleV reviewed Nov 21, 2016

View reviewed changes

NelleV added status: needs revision and removed status: needs review labels Nov 21, 2016

NelleV merged commit 33a1cad into matplotlib:master Nov 21, 2016

QuLogic changed the title ~~[MRG+1] Add maximum streamline length property.~~ Add maximum streamline length property. Nov 22, 2016

QuLogic removed the status: needs revision label Nov 24, 2016

Uh oh!

Conversation

gzahl commented Feb 25, 2016

Uh oh!

tacaswell commented Feb 25, 2016

Uh oh!

gzahl commented Feb 25, 2016

Uh oh!

tacaswell commented Feb 25, 2016

Uh oh!

gzahl commented Feb 25, 2016

Uh oh!

tacaswell commented Feb 25, 2016

Uh oh!

gzahl commented Feb 26, 2016

Uh oh!

gzahl commented Feb 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 7, 2016

Uh oh!

gzahl commented Mar 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzahl commented Nov 3, 2016

Uh oh!

afvincent commented Nov 3, 2016

Uh oh!

tacaswell commented Nov 3, 2016

Uh oh!

gzahl commented Nov 4, 2016

Uh oh!

WeatherGod commented Nov 4, 2016

Uh oh!

gzahl commented Nov 4, 2016

Uh oh!

afvincent commented Nov 4, 2016

Uh oh!

gzahl commented Nov 7, 2016

Uh oh!

QuLogic commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzahl Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzahl commented Nov 21, 2016

Uh oh!

QuLogic commented Nov 7, 2016 •

edited

Loading

gzahl Nov 9, 2016 •

edited

Loading

QuLogic Nov 21, 2016 •

edited

Loading