Skip to content

Conversation

@SimonPeatman
Copy link
Contributor

This is my first ever pull request so apologies if I've done anything wrong!

Further to #2355, I've added support for quiver in iris.plot.

The difference between quiver and most other plotting functions is that it requires two Cubes not one. Therefore, I have changed the functions _map_common and _draw_2d_from_points (and, for consistency, _draw_2d_from_bounds) so they can be given a CubeList (or just a list) of Cubes. All Cubes in the list (which could still have length 1, of course) are then passed as separate arguments to plt.[plotting_function].

Co-ordinates are always taken from the first Cube in the list, but I have included a sanity check (_check_cubes_for_consistency) which is always called to make sure the given Cubes have the same shape and consistent co-ordinates anyway.

@DPeterK
Copy link
Member

DPeterK commented Feb 13, 2017

Thanks @SimonPeatman! We will try and get a look at this soon.

@DPeterK
Copy link
Member

DPeterK commented Feb 24, 2017

Hi @SimonPeatman - again thanks for raising this PR. My main comment on this PR is that it has no tests associated with it. You've introduced a fair bit of new functionality that is currently untested, so I wonder if you could fix that. On the plus side, this change hasn't broken any existing functionality!

I think you have a coding standards error too currently - that's why the Travis build is failing below. If you click the 'details' link to the right of 'The Travis CI build failed' and scroll to the bottom of the webpage that link takes you to you'll find an error that describes the change you need to make.

* placeholder:
  Revise naming + add a usage description.
  Use IrisTest in place of unittest.TestCase everywhere.
  Add test execution timings.
  Make assertMaskedArrayXxx work with a nonmasked result. (#2361)
  Fix 1-dim label data (#2360)
  Update README.md
@SimonPeatman
Copy link
Contributor Author

I've corrected the lines of code which were causing the checks to fail (some lines were too long).

I'm afraid I'll need some guidance when it comes to creating tests since a) I've never done this before and don't really know what I'm doing, and b) the documentation on "Adding a new graphics test" is currently missing: http://scitools.org.uk/iris/docs/latest/developers_guide/graphics_tests.html#adding-a-new-graphics-test

Presumably I need to write tests for iplt.quiver and iplt._check_cubes_for_consistency as these are the two new functions I have created. I think the other changes I've made would already have caused existing tests to fail if I had broken anything.

@SimonPeatman
Copy link
Contributor Author

Now that work has been completed on getting Iris 2.0 released, I thought I'd bump this PR. The documentation on creating graphics tests is still missing (as I mentioned above) so I'll need some guidance if I'm to write tests as requested by @dkillick.

@DPeterK
Copy link
Member

DPeterK commented Feb 20, 2018

Thanks @SimonPeatman - I'm really sorry we haven't had time to look further at this. My main concern as I remember it is that this change means the general plotting functions now require two inputs, which is a big shift in their API. Of course this change would be (in some way at least) required to add quiver plotting, so doing so is not definitely wrong!

There is some documentation in the Iris developers guide on writing tests; see http://scitools.org.uk/iris/docs/latest/developers_guide/tests.html. This may be a good starting point for you although it does not explain much about writing tests! For writing tests probably a good way to learn is to see what is already being done by the existing tests - particularly the plot tests. See, for example, https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/plot/test_contour.py (the tests for iris.plot.contour). You'd need to add a new module in that directory for iris.plot.quiver.

In the test module for quiver it is a good idea to have one test class for each broad aspect of the functionality, and test methods in each of these classes that address particular behaviour that should or should not happen. I'd use the testing that's already done on contour (and the other plotting functionality in iris.plot) as inspiration for the kind of tests you'll also need to provide. Where you've added new functionality or updated existing functionality (particularly iris.plot._draw_2d_from_points) you'll also need to make sure that that behaviour is tested too - including the change to passing a cubelist, for example.

Hope this helps! I know it's still very abstract...

@pp-mo pp-mo mentioned this pull request Aug 2, 2018
@corinnebosley
Copy link
Member

Hey @SimonPeatman, I just wanted to check how you're getting on with adding tests to this PR.

I noticed that if we merge this change it will alter the API for existing plot functions. This is not a problem (and as @dkillick pointed out, if this change is necessary to enable a function which is not currently available then it is perfectly reasonable), but it does mean that we may not be able to introduce this change to a minor version release of iris.

What I mean by that is that we may have to wait until we are about to release iris 3.0 rather than 2.2, as it will change the way that iris responds to existing user code (we call this a 'breaking change'). I haven't confirmed this with the team yet, but that is my understanding of things.

What it does mean is that we have plenty of time to walk you through adding tests if you're a bit confused. Please feel free to ask for more help, we will be happy to assist.

@corinnebosley
Copy link
Member

@pp-mo I suspect it's worth you taking a look at this; I know you have done some work in this area.

@pp-mo
Copy link
Member

pp-mo commented Jan 31, 2019

Hi @SimonPeatman don't know if you still have interest ?
Thanks for the input.
We ended up addressing this in a slightly different way when supporting 2d coord plotting :
introduced here
merged as part of this PR

A Side Note : I had hoped this would also deliver 2d 'streamline' plots, but it turns out that matplotlib does not in fact support this, see : this comment

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.

4 participants