Skip to content

Conversation

@bjlittle
Copy link

@bjlittle bjlittle commented Aug 9, 2022

🚀 Pull Request

Description

Hey @krikru,

As promised, here's some updates that's targeting your patch-1 branch, which is the source of the pull-request SciTools#4894.

It basically adds a suitable entry to our documentation whatsnew, plus we blow the trumpet that it's your first contribution to iris 😄

I've also added a couple of tests to exercise the change in behaviour, which is based on a pull-request (now closed and unmerged) by @rcomer, see SciTools#4136.

To be transparent and fair, I've also acknowledged @rcomer's contribution in the whatsnew along side yourself.

Here is proof that the ci-tests are passing for this change from my fork of iris.

If you're happy with my suggestions, then simply merge this pull-request, and it will automatically become part of your associated pull-request on iris.

At that point, I think we're good to bank this goodness in time for the iris 3.3 release 👍


Consult Iris pull request check list

@krikru
Copy link
Owner

krikru commented Aug 9, 2022

@bjlittle The tests look good to me. Maybe you could also do self.assertIs(gcs.colorbar.ax, ax1) in test__with_axes (I guess a similar line could be added in test__without_axes, but that requires _plot to also return the Axes object that is returned by fig2.add_subplot(1, 1, 1))? I'm just assuming that gcs.colorbar.ax should be the Axes object you specified when calling qplt.contourf (if you do specify one), but maybe it's not.

It looks like test__with_axes is the test that checks for the bug. Have you tested that it triggers the bug that I described when you don't have the fix, and that it doesn't trigger it when you do have the fix?

@bjlittle
Copy link
Author

bjlittle commented Aug 9, 2022

Hey @krikru,

I like your thinking... but just for clarity, the gcs.colorbar.ax isn't the axes that you think it might be, it's an entirely different axes, but I totally get your intent.

So I've pushed a change which does as you suggested, but the axes information that we need is squirreled away somewhere differently within matplotlib. However, with a little bit of detective work it was reasonably straight forward to understand the internal plumbing.

And yeah, test_quickplot.TestSubplotColorbar.test__with_axes is specifically targeting your fix. Whereas test_quickplot.TestSubplotColorbar.test__without_axes is testing the status-quo (aka default behaviour).

Consequently, when I remove your fix to iris.quickplot._label the test_quickplot.TestSubplotColorbar.test__with_axes fails, as we might expect 👍

@bjlittle
Copy link
Author

bjlittle commented Aug 9, 2022

Also, here is the latest ci-tests run on my repo for the latest change.

Copy link
Owner

@krikru krikru left a comment

Choose a reason for hiding this comment

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

Looks good

@bjlittle
Copy link
Author

bjlittle commented Aug 9, 2022

Just rerunning the tests after the last refactor, see https://github.com/bjlittle/iris/actions/runs/2828606527

@bjlittle
Copy link
Author

bjlittle commented Aug 10, 2022

@krikru Once you merge this, then I'll bank SciTools#4894 👍

@krikru krikru merged commit 90f55c6 into krikru:patch-1 Aug 10, 2022
@bjlittle bjlittle deleted the add-whatsnew-and-test-coverage branch November 9, 2022 09:16
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.

2 participants