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

Add streamplot axes method to plot streamlines #664

Merged
merged 23 commits into from Feb 27, 2012

Conversation

tonysyu
Copy link
Contributor

@tonysyu tonysyu commented Jan 8, 2012

Addresses Issue #366

* Add faster adaptive-step algorithm (RK12) to replace RK45 and RK4 integrators.
* Fix arrow placement when streamline is only 2 points.
* Calculate stepsize based on grid size.
Checking that the coordinate less than the array Nx/Ny does not work when the coordinate is float (e.g. it can be between `Nx` and `Nx - 1`).
* RK12 is 1.5--2x faster than RK4 and RK45 produces visually-similar results.
* Remove implementations of RK4 and RK45.
@mdboom
Copy link
Member

mdboom commented Jan 12, 2012

I haven't done a detailed review, but on first glance, I think it would be nice to have a pyplot method for this as well.

@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 12, 2012

I agree. I mentioned this in an email to the dev list when I posted the pull request. When I looked at the pyplot module, I noticed that functions had comments that they were autogenerated by boilerplate.py, and it wasn't clear to me how to do that.

@WeatherGod
Copy link
Member

it means that you run boilerplate.py to regenerate the pyplot.py file and commit the changes to it that way.

@WeatherGod
Copy link
Member

Also note that you should add an entry to the CHANGELOG file

Streamline plotting like Mathematica.
Copyright (c) 2011 Tom Flannaghan.

Permission is hereby granted, free of charge, to any person obtaining a copy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the copyright notice should go in a separate string, not in the module docstring. The module docstring could refer to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but I'm not sure exactly what you mean. Should I move the notice to a separate string (or comment?) in the same module, or should I point to a copyright notice that lives in another file? Also, are you referring to both the "Copyright (c)..." and "Permission is hereby..." parts, or just one of these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there is no single standard way to do it. pyparsing_py2.py puts it in a comment. There are a few other copyright notices sprinkled around other modules, but not many. mlab.py has a free-standing multi-line string acting as if it were a comment. You could ask Tom what his preference is. The two best candidates seem to me to be the comment block (as in pyparsing_py2.py), and a multi-line string that acts like a comment but is not part of the module docstring. In either case, the "Copyright" and "Permission" parts normally go together.

A nice convention might be to have a standard name for any copyright info that differs from the global mpl copyright and license info:

copyright = """Copyright 2012, Joe Smith
Permission...
"""

Then it would be easy to sweep through a package and find all copyright information. I am not aware of any such convention being already established, however. You could start one...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be removed. I'm not at all bothered about the copyright - I only added it because on the mailing list someone requested clarification for a previous script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input Tom. I've removed the copyright notice in the most recent commit.

@efiring
Copy link
Member

efiring commented Jan 16, 2012

In boilerplate.py, you need to add your axes method to the _plotcommands list, and add an appropriate entry to the cmappable dictionary to set the current image appropriately. Then you run boilerplate.py and capture its stdout. Manually edit pyplot.py, deleting the part provided by the previous boilerplate.py, and substituting the new output. Test the result. Now you are ready for a commit. It's all very clumsy, but it doesn't need to be done often.

@efiring
Copy link
Member

efiring commented Jan 16, 2012

A limitation of this code is that is has no support for masked arrays; everything has to be a complete quadrilateral grid. Since most of mpl does support masked arrays, this unfortunate limitation probably should be highlighted in the docstring. (Any thoughts on what it would take to support masking? It is critical for many applications in fluid mechanics, both for engineering and for oceanography.)

@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 16, 2012

@efiring Thanks for the detailed instructions for adding pyplot commands. I think this raises a possible issue with this streamplot implementation: The streamplot command doesn't currently return a ScalarMappable instance. A line collection and individual arrow patches are added separately to the axes. I'm guessing I can collect the FancyArrowPatches into a PatchCollection, but that still leaves me with two separate collections. I'm not really sure what to do here.

Is there any reason pyplot isn't implemented as a subpackage (instead of a single file)? If it were a subpackage the output of boilerplate.py could be piped directly to a file (instead of being pasted into a file), and that file could be imported by the subpackage.

@WeatherGod
Copy link
Member

don't forget about the Legend containers. What should be plotted in the legend for this plot?

@efiring
Copy link
Member

efiring commented Jan 16, 2012

Contouring is similar, in that it is color-mappable, but doesn't return a simple Collection. The streamplot could be handled in a similar manner, with considerable reworking. One option is to leave that out for now, label streamplot "experimental", note that its return value is likely to change, and that at present it is not a proper mappable. One of the consequences is that the colorbar command won't work with it.

@efiring
Copy link
Member

efiring commented Jan 16, 2012

Regarding your pyplot-as-submodule suggestion: I don't think this is even required. The boilerplate output could go to _pyplot_auto.py, and "from _pyplot_auto import *" could be used in pyplot.py to pull it in. But this is getting off-topic, and more suitable for the devel mailing list.

As a temporary fix, streamplot returns the last plotted streamline and arrow for use with colorbars and legends. Instead, all lines should instead be grouped into a single LineCollection, and all FancyArrowPatches should be grouped into a patch collection.
@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 16, 2012

@efiring For the color-mappable, I ended up returning the last-plotted streamline and arrow, and use only the streamline as the color-mappable. A future fix will collect all the lines into a single line collection so that a colorbar would be properly scaled.

@WeatherGod This change plots a color patch in the legend---not ideal, but this is the same behavior as quiver.

@@ -2623,7 +2623,7 @@ def stem(x, y, linefmt='b-', markerfmt='bo', basefmt='r-', hold=None):
if hold is not None:
ax.hold(hold)
try:
ret = ax.stem(x, y, linefmt, markerfmt, basefmt)
ret = ax.stem(x, y, linefmt, markerfmt, basefmt, bottom, label)
draw_if_interactive()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes to stem were auto-generated when I ran boilerplate.py.

Streamplot no longer returns an arrow patch and, instead, only returns a dummy LineCollection.
Note that masked arrays are converted to float and filled with NaNs. Filling integer masked arrays with zeros would also work, but would give incorrect results in some instances.
@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 17, 2012

@efiring In the most recent commit, I added support for masked arrays and NaN values in arrays. I wasn't certain how to handle integer masked arrays, so I just convert to float and fill with NaNs. I'm not certain that's optimal.

@WeatherGod
Copy link
Member

The SOP is to convert NaNs into masked elements and to design algorithms
around masked arrays. This will help keep compatibility with future
changes to masked arrays such as numpy's NA.

Instead of changing masked values to NaNs, change NaNs to masked values.
@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 17, 2012

@WeatherGod Thanks for the advice. I changed the behavior of NaNs and masked arrays as suggested.

As Tom Flannaghan suggested in GitHub discussions, the copyright notice was removed from streamplot.py. Added attribution to CHANGELOG.
Previously, `streamplot` just returned a dummy LineCollection so that colorbars and legends were properly set. Bonus: moving the creation of LineCollection objects out of the loop boosted the speed by about 10%.
@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 19, 2012

Update: I fixed the previous hack for getting colorbars and legends to work properly. (streamplot now returns the actual streamlines as a LineCollection). Also, I added an example for masked arrays.


def valid_index(self, xi, yi):
"""Return True if point is a valid index of grid."""
return xi >= 0 and xi <= self.nx-1 and yi >= 0 and yi <= self.ny-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this could be simplified to 0 <= xi <= self.nx - 1 and 0 <= yi <= self.ny - 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same thing, but since xi and yi can be float values, xi and yi can be larger the nx-1 and ny-1 but less than nx and ny. I think the use of this function changed at some point so the name valid_index is misleading (since an index should be an integer). I'll probably change this in a future commit.

@pelson
Copy link
Member

pelson commented Feb 7, 2012

I should add, I am not a matplotlib developer & my comments hold no authority, but I was sucked in by the pull request and have used this streamlines code to good effect in the past (before this pull request). Hope the devs don't mind :-)

@tonysyu
Copy link
Contributor Author

tonysyu commented Feb 7, 2012

@PhilipElson Thanks for your comments and for reading through the code. There tend to be more Pull Requests than there is spare time to read them; so I'm sure the devs are happy for your help. I definitely appreciate it.

`valid_index` suggests that inputs are integer, which is not true.
__all__ = ['streamplot']


def streamplot(axes, x, y, u, v, density=1, linewidth=1, color='k', cmap=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the call signature, I would like to avoid using explicit defaults for things like linewidth and color. Instead, I would rather that the default be set to None and if they are None, then grab the rcparam values. Don't know if there are defaults for arrowsize and arrowstyle, but maybe one should added?

Even better would be if the color (when None) is pulled from the existing color cycling mechanism! But I would be happy with just pulling the color from rcparams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I changed the defaults to None and use the rc parameters if None. I decided to use the color cycling mechanism (is there a better way than calling axes._get_lines.color_cycle.next()?) .

There doesn't appear to be arrowsize or arrowstyle defaults.


Parameters
----------
x, y : 1d arrays
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it only possible to supply x and y evenly spaced 1d arrays for the grid? Is this a limitation of the algorithm? It would be very nice, if arbitrary 2d arrays for non evenly spaced grids could be supported like for quiver plots. (nice work btw.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting use to evenly spaced grids is required because interpolation
for trajectory integration is implemented using float to integer
conversion (this is the only way I could get acceptable performance in
pure python).

Interpolating the data onto an even high resolution grid before
trajectory integration might be a workaround, although care would be
needed to handle low resolution or very uneven data well (I am uneasy
about this). Obviously the ideal solution would be to write a fast
interpolation routine in C and retain the original grid.

On 23 February 2012 17:05, Manuel Jung
reply@reply.github.com
wrote:

+"""
+import numpy as np
+import matplotlib
+import matplotlib.patches as mpp
+
+
+all = ['streamplot']
+
+
+def streamplot(axes, x, y, u, v, density=1, linewidth=None, color=None,

  •               cmap=None, arrowsize=1, arrowstyle='-|>', minlength=0.1):
  •    """Draws streamlines of a vector flow.
    +
  •    Parameters
  •    ----------
  •    x, y : 1d arrays

Why is it only possible to supply x and y evenly spaced 1d arrays for the grid? Is this a limitation of the algorithm? It would be very nice, if arbitrary 2d arrays for non evenly spaced grids could be supported like for quiver plots. (nice work btw.)


Reply to this email directly or view it on GitHub:
https://github.com/matplotlib/matplotlib/pull/664/files#r481081

@jdh2358
Copy link
Collaborator

jdh2358 commented Feb 26, 2012

This looks like a well reviewed PR and a nice piece of code. @WeatherGod , do you want to do the honors and merge this?

* Surround parameter names with asterisks (e.g. *param*)
* Import `patches` instead of `mpp`
* Use `cbook.is_numlike` instead of `isinstance`.
* Fix outdated docstring.
@WeatherGod
Copy link
Member

Gladly. Great work Tony!

@WeatherGod WeatherGod merged commit 6ca72da into matplotlib:master Feb 27, 2012
@tonysyu
Copy link
Contributor Author

tonysyu commented Feb 27, 2012

Awesome! Thanks to every one for your suggestions, and especially @tomflannaghan for the original implementation.

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.

None yet

8 participants