Contourf bugfix in the presence of nans

[MHBalsmeier]

🚀 Pull Request

Description

Fixes #4261 by using np.nanmax(cube.data) instead of cube.data.max().

[MHBalsmeier]

I have checked, it does not break the problem mentioned in #4086.

[MHBalsmeier]

I don't think I can get the performance test to pass.

[jamesp]

Thanks for the PR fix @MHBalsmeier. Don't worry about the performance regression, that CI test is still work in progress, no reason this change should affect the benchmark that triggered. I've re-run the check.

@rcomer, do you want to just confirm you're happy with this fix? Seems like a reasonable change to me 👍

[rcomer]

Yes thanks @MHBalsmeier for this. Nice detective work! I have to admit that nans had not even crossed my mind when I wrote the original fix. The proposed change looks good to me 👍

[MHBalsmeier]

Okay I think this is ready for the final review.

[rcomer]

I’ve added this to the v3.1 project, as we clearly don’t want to introduce the bug in the next release!

My only question is whether this should have a new test, or whether that would be overkill for such a minor change. If we did want a test, I think it could follow the same pattern as the one I added at #4150. Just need to insert a nan somewhere in cube.data and choose the levels so that the loop is triggered. Then ax.collections should contain LineCollection instances, as they are added by contour. It might be enough to just check that the last element of ax.collections is a LineCollection.

What do you think @MHBalsmeier, @jamesp?

[MHBalsmeier]

I am not sure if I get your point correctly because why would we introduce a bug with this ... I thought this solves #4086 as well as #4261.

As for the test, why not, but I never ran these tests and you seem to have the better idea of how to test this ...
I personally do not think this is really necessary.

[rcomer]

@MHBalsmeier sorry I meant the bug that I introduced at #4150, that you are now fixing. We need to include your fix for the v3.1 release.

[MHBalsmeier]

@rcomer okay I see

[rcomer]

We have now added a test. I verified that the new test fails against main. The new setUp and tearDown ensure that the two tests on the class don't plot on the same figure/axes and interfere with each other's results.

The new failures appear to be because an external file has moved. I have opened #4266 to address that.

[jamesp]

Thanks for fixing the doc failures @rcomer. @MHBalsmeier this just needs a rebase and should be good to go 👍

[MHBalsmeier]

Okay, lets see if I did that correctly.

[jamesp]

Thanks so much @MHBalsmeier for sticking with it and getting this fix in, much appreciated!