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 barbs plot #3710

Merged
merged 18 commits into from
Aug 10, 2021
Merged

Add barbs plot #3710

merged 18 commits into from
Aug 10, 2021

Conversation

jonseddon
Copy link
Contributor

For a personal project it was useful to be able to plot wind barbs. matplotlib.pyplot.barbs does this easily and so I've added it to iris.plot in exactly the same way that quiver is already in there. I've added a gallery example too.

From looking at #3398 I think that I need to add a line to test the image produced to tests/results/imagerepo.json but all the tests are passing locally currently and I can't work out the hash value to include.

@jonseddon
Copy link
Contributor Author

Now that I've seen how Travis runs the tests on the gallery images I think that I can see what I need to do to get the hash and add the image to SciTools/test-images-scitools. I'll do that later this week.

@pp-mo
Copy link
Member

pp-mo commented May 18, 2020

I can see what I need to do to get the hash and add the image to SciTools/test-images-scitools

There is some well-hidden advice about this in the Developer Guide.
In fact, we recently made an improved version, which you can find the in the latest master (unreleased) documentation : available, as usual, from the "docs: latest" badge on the main Readme ...
https://scitools-docs.github.io/iris/master/developers_guide/graphics_tests.html#developer-graphics-tests

The last sections should be of some help : Basically, you can use "iris/tests/idiff.py" to calculate a hash, fix up the repodata.json etc.

Re-reading this though, I can see that it could still be improved !
The procedures listed on the page are now correct, I hope, but there are some awkward + fussy details which mean some procedures need to be followed exactly to get graphics testing and idiff to run properly : creating an extra temporary directory; running from the right directory, finding and identifying files which it re-names, etc.

@jonseddon
Copy link
Contributor Author

Hi @pp-mo, sorry, this is a home project for the sailing club and so I've only just had a chance to look at this again. Thanks for the new v3 graphics tests instructions, they're a great improvement over the v2.4 ones on the website. SciTools/test-iris-imagehash#33 contains the new hash. All tests are now passing apart from iris.tests.test_image_json.TestImageFile which should pass once the PR in the other repository has been merged.

@jonseddon
Copy link
Contributor Author

Just giving this a quick bump. Now that SciTools/test-iris-imagehash#33 has been merged then if an admin could trigger the tests then hopefully they'll pass.

@trexfeathers
Copy link
Contributor

trexfeathers commented Jun 17, 2020

Hi @jonseddon , thanks for keeping an eye on this. Please could you rebase your branch onto the latest Iris master? travis.yml has recently needed to change to point at the latest commit hash for iris-test-data, which is why a couple of tests are still not passing. Thanks

@jonseddon
Copy link
Contributor Author

Thanks @trexfeathers for looking at that. It's been rebased and the tests are passing now.

@trexfeathers
Copy link
Contributor

OK thanks @jonseddon . We're currently looking in general at the docs and gallery examples so I believe @abooton is going to schedule this for some wider team attention 👍

@jonseddon
Copy link
Contributor Author

jonseddon commented Sep 14, 2020

This is just a bit of a bump to see what's going on. Sadly this PR's not on the Kanban board for Iris 3.0 ☹️

Please let me know if you need the gallery example included with this PR updated for the new style gallery that you've introduced.

@bjlittle
Copy link
Member

Hey @jonseddon, your PR needs a wee bit of love... fancy resolving your conflicts with master? Which has moved on quite a bit 😃

@jonseddon
Copy link
Contributor Author

Tests should pass when #3942 is merged in.

@bjlittle
Copy link
Member

bjlittle commented Feb 9, 2021

@jonseddon This PR has sadly gone rather stale... do you have capacity to rebase and resurrect it?

@jonseddon
Copy link
Contributor Author

@bjlittle Wow team scitools has been busy lately! Rebased and hopefully ready for review.

@jonseddon
Copy link
Contributor Author

Needs updating (and new image hashes supplied) as per #4110.

@jonseddon jonseddon marked this pull request as draft May 4, 2021 16:17
@pp-mo
Copy link
Member

pp-mo commented Jul 30, 2021

HI @jonseddon
Sorry for huge delays on this.
I see you have said it's good, but have left it in draft.
Can you confirm that this is this good to go, and if not can we get it finished within the next ~2 weeks ?
We have it earmarked for 3.1.0 release, so it would be really good to get this signed off.

@jonseddon
Copy link
Contributor Author

@trexfeathers I've rebased (after several attempts) and addressed the other comments.

I'm a bit stuck on "Align your new code with the latest CI standards". Which file should I do this in? The other gallery examples don't include this.

There's some new image hashes for the vector tests to be merged in SciTools/test-iris-imagehash#50 so tests should fail this time.

@trexfeathers
Copy link
Contributor

I'm a bit stuck on "Align your new code with the latest CI standards". Which file should I do this in? The other gallery examples don't include this.

My general point was that rebasing onto code where the CI standards had changed wouldn't have adjusted any new code, so there may have been non-compliances with the latest Black, flake8, or especially isort given this is new. Looks like the pre-commit bot caught the only problem (7a8808d).

Regarding # isort:skip: see #3710 (comment)

jonseddon and others added 3 commits August 10, 2021 14:44
Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com>
* 'barbs' of github.com:jonseddon/iris:
  isort:skip
  [pre-commit.ci] auto fixes from pre-commit.com hooks
@trexfeathers
Copy link
Contributor

As far as I'm concerned this is ready to go. @jonseddon are you OK to take it out of draft?

@jonseddon jonseddon marked this pull request as ready for review August 10, 2021 14:13
@trexfeathers trexfeathers merged commit 619c62c into SciTools:main Aug 10, 2021
@trexfeathers
Copy link
Contributor

Thanks for your patience @jonseddon 🏆

@jonseddon jonseddon deleted the barbs branch August 10, 2021 15:00
@jonseddon
Copy link
Contributor Author

Thanks for your patience @jonseddon trophy

Thanks to yourself and everyone who's helped!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants