Implementation of 3D quiver

[weichensw]

Hi,

We are a small team of students from University of Toronto. Our instructor asked as to implement a MPL feature as one of our project :) so we choose the 3D quiver plot, as mentioned in #1026 and mailing list

We only had a week to work on the implementation and term report, so it's not a full featured quiver as 2D quiver. But we got the basics working and it should be usable.

Below is a graph for z = sin(x) + cos(y)

[tacaswell]

I think our (possibly over-zealous) pep8 tests will complain about the lack of a new line.

[weichensw]

new lines are in place :)

[tacaswell]

Cool! I like this idea of assiging adding features to OSS porjects as homework 😄

I left some nit-picky/style comments.

@WeatherGod Thoughts?

[WeatherGod]

This is absolutely fantastic!

I will spend some time tonight reviewing the code.

On Tue, Apr 8, 2014 at 4:50 PM, Thomas A Caswell
notifications@github.comwrote:

Cool! I like this idea of assiging adding features to OSS porjects as
homework [image: 😄]

I left some nit-picky/style comments.

@WeatherGod https://github.com/WeatherGod Thoughts?

Reply to this email directly or view it on GitHubhttps://github.com//pull/2961#issuecomment-39900171
.

[WeatherGod]

use np.broadcast_arrays(). You get the additional benefit of making sure everything is broadcastable, so you can eliminate the shape check later

[weichensw]

At first I didn't use broadcast_arrays() because it strip off the mask of masked arrays. But I think it should be preferred to be consistent with rest of APIs. Used a alternative way to handle the masked array so that broadcast can be used too.

[WeatherGod]

Here is a quick-n-dirty masked array version of broadcast_arrays(). It is very efficient and very clean and easy to understand.

def ma_broadcast_arrays(*arrays):
    arrays = [np.ma.masked_array(x) for x in arrays)]
    broadcasted = np.broadcast_arrays(*arrays)
    broadcasted_masks = np.broadcast_arrays(*[x.mask for x in arrays])
    return [np.ma.masked_array(x, mask=mask) for x, mask in zip(broadcasted, broadcasted_masks)]

The beauty of the above function is that the inputs can be heterogenous (scalars, regular numpy arrays, masked arrays), and everything comes out at the end as broadcasted views of masked arrays so there is little if any copies.

Then, you can use cbook.delete_masked_points() to get rid of anything masked, so you don't have to worry about making the mask the same across all arrays.

[weichensw]

Wow...found lot of useful functions in cbook :) great thanks for the pointer.

As for this case, I think the masks must be broadcasted together with data, if broadcasted separately, the outcome mask and data will not match each other in some scenario, consider:

arrays = [
    np.ma.array([1, 2, 3], mask=[1, 0, 0]),
    np.array([[4, 5, 6], [7, 8, 9]])
]

Line 2522-2526 is to address this scenario.

When picking the elements, the approach I used here happened to be very similar to delete_masked_points(), the main difference is delete_masked_points() is doing and to reduce the mask, here I am doing or to reduce the mask. I think it make sense to do so, if I use and here, the input parameter will need to make sure all u,v,w,x,y,z masked in the same spot after broadcasting in order for a position to be masked; using or, the user can simply mask any of the u,v,w,x,y,z and knowing the position is masked no matter if the mask matches or not after broadcasting in rest of parameters. If one got multiple masks and feel confused, one can simply strip all the masks and only mask on one parameter.

Different than delete_masked_points(), here since I know that the arrays and mask are in same shape, I can simply apply the mask and use compressed() to get rid of the masked elements without additional checking like delete_masked_points() needed. Also I get the added benefit of being able to handle masked array of more than 1-D.

[weichensw]

Thanks for the insightful comments!
Made some adjustments according to the comments given.

[pelson]

👍 - this is an excellent piece of work with a test to boot - great stuff! Once @WeatherGod has had a good read, I'm in favour of this change.

@xbtsw and collaborators: Please feel free to add your names and the feature to `doc/users/whats_new.rst`` and get your name in lights. I wouldn't find it inappropriate to also list your institution in the what's new if your instructor so wishes.

Thanks for the contribution, and I hope you've enjoyed it sufficiently to consider getting your teeth stuck into other matplotlib features/bugs.

[WeatherGod]

Sorry I haven't responded on this. I have not forgotten about this. I want to look into the cbook.delete_masked_points() function, as I don't think it should be behaving as you describe. I would like to get delete_masked_points() to be used here like it is used elsewhere and would greatly simplify the code here.

[weichensw]

Hi @WeatherGod thanks for your reply :)
In fact I only read through the code of delete_masked_points() but didn't thoroughly tested it out.
I made a quick comparison test between delete_masked_points() and the method I used. Turns out my understanding for delete_masked_points() was off. I posted the script below in case you found it useful.

Also while doing this, I found a bug in the code, so I pushed a new commit to fix it, also added to whats_new.rst file :)

import numpy as np
from matplotlib import cbook


def quiver_broadcast(arrays):
    input_args = arrays
    argi = len(arrays)
    ### below are copied from quiver
    # if any of the args are scalar, convert into list
    input_args = [[k] if isinstance(k, (int, float)) else k for k in input_args]
    # extract the masks, if any
    masks = [k.mask for k in input_args if isinstance(k, np.ma.MaskedArray)]
    # broadcast to match the shape
    bcast = np.broadcast_arrays(*(input_args + masks))
    input_args = bcast[:argi]
    masks = bcast[argi:]
    if masks:
        # combine the masks into one
        mask = reduce(np.logical_or, masks)
        # put mask on and compress
        input_args = [np.ma.array(k, mask=mask).compressed() for k in input_args]
    else:
        input_args = [k.flatten() for k in input_args]
    return input_args

def quiver_alt(arrays):
    # Alternative implementation trying to use delete_masked_points()
    input_args = arrays
    # TODO: How to support broadcasting?
    # if any of the args are scalar, convert into list
    input_args = [[k] if isinstance(k, (int, float)) else k for k in input_args]
    # convert to ndarray, so can be flattened in next step
    input_args = [np.array(k) if not isinstance(k, np.ndarray) else k for k in input_args]
    # flatten since delete_masked_points() only accept 1-D arrays
    input_args = [k.flatten() for k in input_args]
    return cbook.delete_masked_points(*input_args)

def test(arrays):
    print "input"
    print "====="
    print arrays
    print "quiver_broadcast:"
    print "================="
    try:
        ret = quiver_broadcast(arrays)
        print ret
    except Exception as ex:
        print "Exception: %s" % ex

    print "delete_masked_points:"
    print "====================="
    try:
        ret = cbook.delete_masked_points(*arrays)
        print ret
    except Exception as ex:
        print "Exception: %s" % ex

    print "quiver_alt:"
    print "====================="
    try:
        ret = quiver_alt(arrays)
        print ret
    except Exception as ex:
        print "Exception: %s" % ex
    print "~~~~~~~~~~~~~~~~~~~~~~~~~~"

if __name__ == '__main__':
    test([
        np.ma.array([1, 2, 3], mask=[1, 0, 0]),
        np.array([1, 2, 3]),
    ])
    test([
        np.ma.array([1, 2, 3], mask=[1, 0, 0]),
        np.ma.array([1, 2, 3], mask=[1, 0, 0]),
    ])
    test([
        np.ma.array([1, 2, 3], mask=[1, 0, 0]),
        np.array([[1, 2, 3], [4, 5, 6]]),
    ])
    test([
        np.ma.array([1, 2, 3], mask=[1, 0, 0]),
        np.ma.array([[1, 2, 3], [4, 5, 6]], mask = [[1, 0, 0], [1, 0, 0]]),
    ])
    test([
        np.ma.array([[1, 2, 3], [4, 5, 6]], mask = [[0, 1, 0], [0, 0, 0]]),
        np.ma.array([[1, 2, 3], [4, 5, 6]], mask = [[1, 0, 0], [0, 0, 0]]),
    ])

[WeatherGod]

Make sure you have a newline at the end of the file

[WeatherGod]

Pinging on adding a newline at the end of the file.

[WeatherGod]

At about this point, I would do a check for empty arrays and do an early return if that is the case. I can easily imagine a case of inputs being all masked out.

[WeatherGod]

Need two blank lines after this

[WeatherGod]

Once the remaining comments are addressed, I am ok with merging this to get it into 1.4.0. I might make it a future PR to add a cbook.broadcast_any_arrays() and to improve cbook.delete_masked_points() such that it could be used here and elsewhere.

[TheInvoker]

Hi WeatherGod,

We will gladly make these changes. Our team is just finishing up final exams this week and hope to commit these changes early next week.

[WeatherGod]

Ping?

I hope the finals went well for you all.

[tacaswell]

@TheInvoker What is the state of this? @WeatherGod Can you be responsible for getting this ready to merge (looks like it needs a re-base at a minimum)?

[WeatherGod]

I guess I could. I guess I would just have to fork their branch and send a new PR with the additional changes?

[tacaswell]

@WeatherGod I mostly meant taking care of prodding the OP to make sure it happens, but your approach would also work.

[tacaswell]

Closing in favor of #3149